Removed the context menu in kiosk mode

Removed the context menu in kiosk mode

This is my first change request so...

BUG=475506
TEST=Start Chrome with --kiosk and the context menu most not be shown.
Start Chrome without the --kiosk flag and the context menu must be shown.

Committed: https://crrev.com/25e2a6ce5742821aedf91746804487ae64ce2190
Cr-Commit-Position: refs/heads/master@{#337741}

[frederic.jacob.78, 2015-04-26 01:49:27]

frederic.jacob.78@gmail.com changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2015-04-29 21:17:15]

https://codereview.chromium.org/1091173005/diff/1/chrome/browser/ui/views/ren...
File
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
(right):

https://codereview.chromium.org/1091173005/diff/1/chrome/browser/ui/views/ren...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:216:
if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
Pretty simple fix.  I'll test this locally to make sure it does the right thing.

This only fixes the problem on views; I'm not sure whether --kiosk works or not
on Mac/CrOS/etc.  If so we'd want to fix this there as well.  Do you have a Mac
you can test that on?  If not I can try and scrounge one.

[frederic.jacob.78, 2015-04-29 21:47:27]

Yes I have a Mac that I dual boot with Ubuntu, right now it is not setup to
build Chromium! I will begin by testing the --kiosk flag on my Mac and I
will also setup building Chromium on it, it can be useful later ;)



On Wed, Apr 29, 2015 at 5:17 PM, <pkasting@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1091173005/diff/1/chrome/browser/ui/views/ren...
> File
>
>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
> (right):
>
>
>
https://codereview.chromium.org/1091173005/diff/1/chrome/browser/ui/views/ren...
>
>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:216:
> if
> (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
> Pretty simple fix.  I'll test this locally to make sure it does the
> right thing.
>
> This only fixes the problem on views; I'm not sure whether --kiosk works
> or not on Mac/CrOS/etc.  If so we'd want to fix this there as well.  Do
> you have a Mac you can test that on?  If not I can try and scrounge one.
>
> https://codereview.chromium.org/1091173005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[frederic.jacob.78, 2015-04-30 00:51:06]

Ok, I tested it on my Mac and on Windows and the --kiosk flag work. So I
will setup all my systems and I will do the job on each, however I do not
have any way of testing the ChromeOS version!

On Wed, Apr 29, 2015 at 5:47 PM, Frédéric Jacob <frederic.jacob.78@gmail.com
> wrote:

> Yes I have a Mac that I dual boot with Ubuntu, right now it is not setup
> to build Chromium! I will begin by testing the --kiosk flag on my Mac and I
> will also setup building Chromium on it, it can be useful later ;)
>
>
>
> On Wed, Apr 29, 2015 at 5:17 PM, <pkasting@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1091173005/diff/1/chrome/browser/ui/views/ren...
>> File
>>
>>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
>> (right):
>>
>>
>>
https://codereview.chromium.org/1091173005/diff/1/chrome/browser/ui/views/ren...
>>
>>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:216:
>> if
>> (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
>> Pretty simple fix.  I'll test this locally to make sure it does the
>> right thing.
>>
>> This only fixes the problem on views; I'm not sure whether --kiosk works
>> or not on Mac/CrOS/etc.  If so we'd want to fix this there as well.  Do
>> you have a Mac you can test that on?  If not I can try and scrounge one.
>>
>> https://codereview.chromium.org/1091173005/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-04-30 08:45:15]

On 2015/04/30 00:51:06, frederic.jacob.78 wrote:
> Ok, I tested it on my Mac and on Windows and the --kiosk flag work. So I
> will setup all my systems and I will do the job on each, however I do not
> have any way of testing the ChromeOS version!

I misspoke, CrOS will be covered by views.  So just views and Mac should be good
enough.

Also, make sure to follow the steps under "Get your code ready - 3. Legal" on
http://dev.chromium.org/developers/contributing-code if this is your first
patch.

THanks again for this!

[Peter Kasting, 2015-05-01 02:26:02]

pkasting@chromium.org changed reviewers:
+ jam@chromium.org, sky@chromium.org

[Peter Kasting, 2015-05-01 02:26:02]

OK, I looked at this more.  I'm adding sky@ as a reviewer for input on basically
everything I say below since he's done more work with context menus than I have.
 Also adding jam@ for a specific question; John, see the content layer issue
below.

First, this doesn't catch all context menus.  For example, if I start in --kiosk
mode on the New Tab page, I can right-click the bookmark bar at the top and get
a context menu.  That's because this is actually native content and not web UI. 
You'll need to disable these sorts of context menus as well.

As to the fix in this patch, I'm torn on whether you should:
* Leave it where it is
* Move it one stack frame up, to ChromeWebContentsViewDelegateViews::ShowMenu()
* Move the --kiosk flag to content_switches.h and hoist this code to
RenderFrameHostImpl::OnContextMenu()

The last option would fix this for Mac as well, but I don't know whether the
content layer folks would object to the idea of --kiosk being a content-level
switch.  John, do you have input here?

The other two options would still only work for views, but would have slightly
different ramifications in terms of what's affected in the next of virtual
function calls/overrides here.  Leaving this where it is would benefit people
who instantiate RenderViewContextMenuViews in more places than just
ChromeWebContentsViewDelegateViews::ShowMenu(); moving it up would benefit other
subclasses of RenderViewContextMenuBase besides this one.  I haven't looked
closely enough into these to truly understand which is better.

[jam, 2015-05-01 16:09:22]

On 2015/05/01 02:26:02, Peter Kasting wrote:
> OK, I looked at this more.  I'm adding sky@ as a reviewer for input on
basically
> everything I say below since he's done more work with context menus than I
have.
>  Also adding jam@ for a specific question; John, see the content layer issue
> below.
> 
> First, this doesn't catch all context menus.  For example, if I start in
--kiosk
> mode on the New Tab page, I can right-click the bookmark bar at the top and
get
> a context menu.  That's because this is actually native content and not web
UI. 
> You'll need to disable these sorts of context menus as well.
> 
> As to the fix in this patch, I'm torn on whether you should:
> * Leave it where it is
> * Move it one stack frame up, to
ChromeWebContentsViewDelegateViews::ShowMenu()
> * Move the --kiosk flag to content_switches.h and hoist this code to
> RenderFrameHostImpl::OnContextMenu()
> 
> The last option would fix this for Mac as well, but I don't know whether the
> content layer folks would object to the idea of --kiosk being a content-level
> switch.  John, do you have input here?

since content itself doesn't put up context menus, but tells the embedder, and
kiosk mode is an application concept (since it's UI), this shouldn't go into
content. i.e. the embedder which knows that it's in kiosk mode should not put up
the menu itself.

> 
> The other two options would still only work for views, but would have slightly
> different ramifications in terms of what's affected in the next of virtual
> function calls/overrides here.  Leaving this where it is would benefit people
> who instantiate RenderViewContextMenuViews in more places than just
> ChromeWebContentsViewDelegateViews::ShowMenu(); moving it up would benefit
other
> subclasses of RenderViewContextMenuBase besides this one.  I haven't looked
> closely enough into these to truly understand which is better.

[frederic.jacob.78, 2015-05-01 22:43:52]

On Fri, May 1, 2015 at 12:09 PM, <jam@chromium.org> wrote:

> On 2015/05/01 02:26:02, Peter Kasting wrote:
>
>> OK, I looked at this more.  I'm adding sky@ as a reviewer for input on
>>
> basically
>
>> everything I say below since he's done more work with context menus than I
>>
> have.
>
>>   Also adding jam@ for a specific question; John, see the content layer
>> issue
>> below.
>>
>
>  First, this doesn't catch all context menus.  For example, if I start in
>>
> --kiosk
>
>> mode on the New Tab page, I can right-click the bookmark bar at the top
>> and
>>
> get
>
>> a context menu.  That's because this is actually native content and not
>> web
>>
> UI.
>
>> You'll need to disable these sorts of context menus as well.
>>
>
>  As to the fix in this patch, I'm torn on whether you should:
>> * Leave it where it is
>> * Move it one stack frame up, to
>>
> ChromeWebContentsViewDelegateViews::ShowMenu()
>
>> * Move the --kiosk flag to content_switches.h and hoist this code to
>> RenderFrameHostImpl::OnContextMenu()
>>
>
>  The last option would fix this for Mac as well, but I don't know whether
>> the
>> content layer folks would object to the idea of --kiosk being a
>> content-level
>> switch.  John, do you have input here?
>>
>
> since content itself doesn't put up context menus, but tells the embedder,
> and
> kiosk mode is an application concept (since it's UI), this shouldn't go
> into
> content. i.e. the embedder which knows that it's in kiosk mode should not
> put up
> the menu itself.
>
>
>
>  The other two options would still only work for views, but would have
>> slightly
>> different ramifications in terms of what's affected in the next of virtual
>> function calls/overrides here.  Leaving this where it is would benefit
>> people
>> who instantiate RenderViewContextMenuViews in more places than just
>> ChromeWebContentsViewDelegateViews::ShowMenu(); moving it up would benefit
>>
> other
>
>> subclasses of RenderViewContextMenuBase besides this one.  I haven't
>> looked
>> closely enough into these to truly understand which is better.
>>
>
>
>
> https://codereview.chromium.org/1091173005/
>

Ok,

So here is what I understand: the code need to be in the embedder (Code
under /Chrome?), I need to remove the context menu into the web page area
and also in other area like the bookmark bar and need to be removed for all
platforms. As for the current change I have made, I need to analyze if it
is better to stay it there or move it into
ChromeWebContentsViewDelegateViews::ShowMenu().

I will work on this over the weekend and try to find something!

P.S. I looked into the "Get your code ready - 3. Legal" and I completed the
CLA. As for the AUTHORS entry, do I need to make the change in the current
CL or do I need to do a single CL for this?

Thanks,

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-05-01 23:02:22]

On 2015/05/01 22:43:52, frederic.jacob.78 wrote:
> So here is what I understand: the code need to be in the embedder (Code
> under /Chrome?), I need to remove the context menu into the web page area
> and also in other area like the bookmark bar and need to be removed for all
> platforms. As for the current change I have made, I need to analyze if it
> is better to stay it there or move it into
> ChromeWebContentsViewDelegateViews::ShowMenu().

This all sounds correct.  I suspect you won't end up wanting to move your code,
but I'm not sure.

> P.S. I looked into the "Get your code ready - 3. Legal" and I completed the
> CLA. As for the AUTHORS entry, do I need to make the change in the current
> CL or do I need to do a single CL for this?

You should add yourself to that file in the current CL.

[frederic.jacob.78, 2015-05-29 20:58:08]

On Fri, May 1, 2015 at 7:02 PM, <pkasting@chromium.org> wrote:

> On 2015/05/01 22:43:52, frederic.jacob.78 wrote:
>
>> So here is what I understand: the code need to be in the embedder (Code
>> under /Chrome?), I need to remove the context menu into the web page area
>> and also in other area like the bookmark bar and need to be removed for
>> all
>> platforms. As for the current change I have made, I need to analyze if it
>> is better to stay it there or move it into
>> ChromeWebContentsViewDelegateViews::ShowMenu().
>>
>
> This all sounds correct.  I suspect you won't end up wanting to move your
> code,
> but I'm not sure.
>
>  P.S. I looked into the "Get your code ready - 3. Legal" and I completed
>> the
>> CLA. As for the AUTHORS entry, do I need to make the change in the current
>> CL or do I need to do a single CL for this?
>>
>
> You should add yourself to that file in the current CL.
>
> https://codereview.chromium.org/1091173005/
>

Hi,

I found some time to look at this and I think I have found something that
work. I also span some time to investigate the UI sub system to try to
block the right click in the event code, but as the kiosk switch is not
available in the UI sub system I stopped my investigation there.

For the content area I decided to block the context menu in
the ChromeWebContentsViewDelegateXXX::BuildMenu because it will save some
allocation. The RenderViewContextMenuXXX is only used by the
ChromeWebContentsViewDelegateXXX and the ChromeWebContentsViewDelegateXXX
only use the RenderViewContextMenuXXX to display context menu. So I
hesitated to keep the code where I first put it and moving it in the
BuildMenu because in the BuildMenu it save memory an CPU time by not
creating the RenderViewContextMenuXXX and not executing the code in it, but
it is more logical to keep it in the RenderViewContextMenuXXX::RunMenuAt
and also do not let a null pointer into the scoped_ptr object. But there is
already a case when we do not create the RenderViewContextMenuXXX in the
BuildMenu. What do you think of that?

I also found two more context menu, one for each App icon in the Apps menu
and one in the little warning bar but only on OSX. I was not sure about
removing them, because they look to be giving needed feature, like removing
the app or text to speak feature in the warning bar. Do you think I should
also remove them?

One last thing, how do I send a new CL? Do I need to do a new commit in the
same branch I used to send the first and send an update of the first or do
I need to send a new CL in a new branch?

Thanks,

P.S. I visited Mtl Google office yesterday and the Googler began talking
about you all!!

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-05-29 23:09:45]

On Fri, May 29, 2015 at 1:58 PM, Frédéric Jacob <frederic.jacob.78@gmail.com
> wrote:

> For the content area I decided to block the context menu in the
> ChromeWebContentsViewDelegateXXX::BuildMenu because it will save some
> allocation. The RenderViewContextMenuXXX is only used by the
> ChromeWebContentsViewDelegateXXX and the ChromeWebContentsViewDelegateXXX
> only use the RenderViewContextMenuXXX to display context menu. So I
> hesitated to keep the code where I first put it and moving it in the
> BuildMenu because in the BuildMenu it save memory an CPU time by not
> creating the RenderViewContextMenuXXX and not executing the code in it, but
> it is more logical to keep it in the RenderViewContextMenuXXX::RunMenuAt
> and also do not let a null pointer into the scoped_ptr object. But there is
> already a case when we do not create the RenderViewContextMenuXXX in the
> BuildMenu. What do you think of that?
>

Worry about code simplicity/readability/robustness, not about efficiency.

I also found two more context menu, one for each App icon in the Apps menu
> and one in the little warning bar but only on OSX. I was not sure about
> removing them, because they look to be giving needed feature, like removing
> the app or text to speak feature in the warning bar. Do you think I should
> also remove them?
>

Not sure; maybe put in code to remove them in your proposed CL and it'll be
easier to see concretely what's at stake.

One last thing, how do I send a new CL? Do I need to do a new commit in the
> same branch I used to send the first and send an update of the first or do
> I need to send a new CL in a new branch?
>

You should just be making more commits to the existing git branch and using
"git cl upload" to upload those commits to the server.  Make sure to send a
new message requesting a re-review from the codereview tool once you're
ready.

PK

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[frederic.jacob.78, 2015-06-13 16:09:43]

I added the changes to disable the context menu in Views and Cocoa for web
content, bookmark, app list icons and infobar for Cocoa. The App list code is
not really good because I did not found any good way to get the kiosk flag from
the Javascript code, but I don't think that we should remove this context menu.
For the Cocoa infobar the context menu is coming from the underling NSTextView.
There is no way to disable the context menu by code so I ended up trapping the
right click event at all time. This one is more problematic because we can go
out of the application with some menu items! At the same time I am not sure we
want to show this context menu at all.

[Peter Kasting, 2015-06-15 20:16:34]

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:282: if (!this.appData_.kioskMode)
Nit: {}

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:284: cr.ui.contextMenuHandler);
Nit: Indenting not correct

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:293: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode)) {
Nit: 80 columns

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:98:
// Don't display the context menu in kiosk mode.
Nit: This comment just restates the code, remove it.  (Multiple files)

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:99:
if(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
Nit: space after "if"

https://codereview.chromium.org/1091173005/diff/20001/ui/base/cocoa/controls/...
File ui/base/cocoa/controls/hyperlink_text_view.mm (right):

https://codereview.chromium.org/1091173005/diff/20001/ui/base/cocoa/controls/...
ui/base/cocoa/controls/hyperlink_text_view.mm:79: [self fixupCursor];
Is this what you meant by trapping the right-mouse click at all times?  It's not
totally obvious to me what this does.

You should get a cocoa OWNER to review this, e.g. rsesek.

[frederic.jacob.78, 2015-06-18 00:41:38]

I looked at the comments and I made changes to reflect all the comments. 

As it is my first review, I am not sure how to do it. Do I need to ack or done
for each comments? Or do I just send the new cl that cover it as an acknowledge?
As for getting rsesek to review the hyperlink_text_view.mm code, I assumed that
adding him to this issues in the Publish reviewers field, was the right way to
do it.

https://codereview.chromium.org/1091173005/diff/20001/ui/base/cocoa/controls/...
File ui/base/cocoa/controls/hyperlink_text_view.mm (right):

https://codereview.chromium.org/1091173005/diff/20001/ui/base/cocoa/controls/...
ui/base/cocoa/controls/hyperlink_text_view.mm:79: [self fixupCursor];
On 2015/06/15 20:16:34, Peter Kasting wrote:
> Is this what you meant by trapping the right-mouse click at all times?  It's
not
> totally obvious to me what this does.
> 
> You should get a cocoa OWNER to review this, e.g. rsesek.

Sorry, Yes that what I call trapping. As I did not found any way in the Cocoa
API to remove the context menu I overloaded the right mouse down handler of the
NSTextView so that the default function did not receive the event and do not
display the context menu. I did not add code to detect the kiosk mode because I
think that it is strange to have this context menu there. As suggested, Robert
Sesek could you review this?

[Peter Kasting, 2015-06-18 00:50:34]

On 2015/06/18 00:41:38, frederic.jacob.78 wrote:
> I looked at the comments and I made changes to reflect all the comments. 
> 
> As it is my first review, I am not sure how to do it. Do I need to ack or done
> for each comments? Or do I just send the new cl that cover it as an
acknowledge?

Normally people go through each comment and use the "Done" button on anything
they've fixed, or the "Ack" button on anything that is now moot (e.g. all the
surrounding code changed).  This is a good way to make sure you actually
responded to each comment.  As a reviewer, if I get a re-review request without
this I don't know whether you believe you've dealt with everything or not.

> As for getting rsesek to review the hyperlink_text_view.mm code, I assumed
that
> adding him to this issues in the Publish reviewers field, was the right way to
> do it.

Yes, but also make sure any time you add a new reviewer to a CL your next change
explicitly asks them to review (and says what/why).  You've done that here, just
make sure to call it out at the top of the review message in the future, so
people will see their names immediately in their email inboxes.

[frederic.jacob.78, 2015-06-19 10:29:25]

frederic.jacob.78@gmail.com changed reviewers:
+ rsesek@chromium.org

[frederic.jacob.78, 2015-06-19 10:36:32]

On 2015/06/18 00:50:34, Peter Kasting wrote:
> On 2015/06/18 00:41:38, frederic.jacob.78 wrote:
> > I looked at the comments and I made changes to reflect all the comments. 
> > 
> > As it is my first review, I am not sure how to do it. Do I need to ack or
done
> > for each comments? Or do I just send the new cl that cover it as an
> acknowledge?
> 
> Normally people go through each comment and use the "Done" button on anything
> they've fixed, or the "Ack" button on anything that is now moot (e.g. all the
> surrounding code changed).  This is a good way to make sure you actually
> responded to each comment.  As a reviewer, if I get a re-review request
without
> this I don't know whether you believe you've dealt with everything or not.
> 
> > As for getting rsesek to review the hyperlink_text_view.mm code, I assumed
> that
> > adding him to this issues in the Publish reviewers field, was the right way
to
> > do it.
> 
> Yes, but also make sure any time you add a new reviewer to a CL your next
change
> explicitly asks them to review (and says what/why).  You've done that here,
just
> make sure to call it out at the top of the review message in the future, so
> people will see their names immediately in their email inboxes.

Thank for the advice! But I think I did not add rsesek correctly. I now added
him to
the reviewer list using the edit issue link.

[Robert Sesek, 2015-06-19 10:59:16]

Can you swallow the event in -[BrowserCrApplication sendEvent:] instead? If that
works, it's better than sprinkling these checks all through the codebase.

[frederic.jacob.78, 2015-06-19 12:03:34]

On Fri, Jun 19, 2015 at 6:59 AM, <rsesek@chromium.org> wrote:

> Can you swallow the event in -[BrowserCrApplication sendEvent:] instead?
> If that
> works, it's better than sprinkling these checks all through the codebase.
>
> https://codereview.chromium.org/1091173005/
>

I tested it rapidly this morning and it look to be working good! I will
analyze it more when I return home after work today.

That was my plan at first to swallow the event but all the handling codes I
found was in /ui, where the kiosk flag is not available, and in /content,
where I was told it is not the place to do it. I must admit that as it is
my first experience with Cocoa I work on Windows and try to replicate the
change in Cocoa.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[frederic.jacob.78, 2015-06-22 11:32:01]

On 2015/06/19 12:03:34, frederic.jacob.78 wrote:
> On Fri, Jun 19, 2015 at 6:59 AM, <mailto:rsesek@chromium.org> wrote:
> 
> > Can you swallow the event in -[BrowserCrApplication sendEvent:] instead?
> > If that
> > works, it's better than sprinkling these checks all through the codebase.
> >
> > https://codereview.chromium.org/1091173005/
> >
> 
> I tested it rapidly this morning and it look to be working good! I will
> analyze it more when I return home after work today.
> 
> That was my plan at first to swallow the event but all the handling codes I
> found was in /ui, where the kiosk flag is not available, and in /content,
> where I was told it is not the place to do it. I must admit that as it is
> my first experience with Cocoa I work on Windows and try to replicate the
> change in Cocoa.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I looked into this over the weekend and it look to be working fine.
This give us a simple and full solution for the removal of the context menu.
The only problem I found is that this do not give us the possibility of keeping
the App list context menu
and this cannot be replicated to other platform resulting in two different
implementation.

So what do you think of that?

[Robert Sesek, 2015-06-22 13:30:37]

On 2015/06/22 11:32:01, frederic.jacob.78 wrote:
> On 2015/06/19 12:03:34, frederic.jacob.78 wrote:
> > On Fri, Jun 19, 2015 at 6:59 AM, <mailto:rsesek@chromium.org> wrote:
> > 
> > > Can you swallow the event in -[BrowserCrApplication sendEvent:] instead?
> > > If that
> > > works, it's better than sprinkling these checks all through the codebase.
> > >
> > > https://codereview.chromium.org/1091173005/
> > >
> > 
> > I tested it rapidly this morning and it look to be working good! I will
> > analyze it more when I return home after work today.
> > 
> > That was my plan at first to swallow the event but all the handling codes I
> > found was in /ui, where the kiosk flag is not available, and in /content,
> > where I was told it is not the place to do it. I must admit that as it is
> > my first experience with Cocoa I work on Windows and try to replicate the
> > change in Cocoa.
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> I looked into this over the weekend and it look to be working fine.
> This give us a simple and full solution for the removal of the context menu.
> The only problem I found is that this do not give us the possibility of
keeping
> the App list context menu
> and this cannot be replicated to other platform resulting in two different
> implementation.
> 
> So what do you think of that?

I think "no context menus" means "no context menus", so I'm fine with the App
List issue. It's OK if the implementations differ between platforms, the
frontend code is completely different.

[frederic.jacob.78, 2015-06-24 18:07:53]

I have corrected the style problems as recommended in the review and I reverted
all the Cocoa code to only use the sendEvent function of the
BrowserCrApplication class. As for the sendEvent change, I am not sure about the
way I have done it, because I created 3 temps variables to build the if. I have
done it to respect the 80 chars. limit and to make the code clearer.

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:282: if (!this.appData_.kioskMode)
On 2015/06/15 20:16:33, Peter Kasting wrote:
> Nit: {}

Done.

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/resource...
chrome/browser/resources/ntp4/apps_page.js:284: cr.ui.contextMenuHandler);
On 2015/06/15 20:16:33, Peter Kasting wrote:
> Nit: Indenting not correct

Done.

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:293: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode)) {
On 2015/06/15 20:16:33, Peter Kasting wrote:
> Nit: 80 columns

Acknowledged.

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:98:
// Don't display the context menu in kiosk mode.
On 2015/06/15 20:16:33, Peter Kasting wrote:
> Nit: This comment just restates the code, remove it.  (Multiple files)

This change have been removed but I have done it for other files.

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:99:
if(base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kKioskMode))
On 2015/06/15 20:16:33, Peter Kasting wrote:
> Nit: space after "if"

Acknowledged.

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc (right):

https://codereview.chromium.org/1091173005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_context_menu.cc:67: // Don't display
the context menu in kiosk mode.
Removed this comment as recommended in another file.

https://codereview.chromium.org/1091173005/diff/20001/ui/base/cocoa/controls/...
File ui/base/cocoa/controls/hyperlink_text_view.mm (right):

https://codereview.chromium.org/1091173005/diff/20001/ui/base/cocoa/controls/...
ui/base/cocoa/controls/hyperlink_text_view.mm:79: [self fixupCursor];
On 2015/06/15 20:16:34, Peter Kasting wrote:
> Is this what you meant by trapping the right-mouse click at all times?  It's
not
> totally obvious to me what this does.
> 
> You should get a cocoa OWNER to review this, e.g. rsesek.

Acknowledged.

[Robert Sesek, 2015-06-24 18:36:12]

I definitely think this is a nicer solution. Thanks!

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:492: const base::CommandLine*
command_line =
nit: blank line before

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:492: const base::CommandLine*
command_line =
naming: commandLine

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:499: if (!kioskMode ||
(!isRightMouseMenu && !isLeftMouseMenu)) {
Leave a comment as to why this is here.

[Peter Kasting, 2015-06-24 20:40:57]

LGTM

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:494: const bool kioskMode =
command_line->HasSwitch(switches::kKioskMode);
Nit: Personally I think the explosion of temps here is a bit excessive.  I would
at least inline |command_line| into |kioskMode|, and I would probably inline
that into the conditional below.  |isLeftMouseMenu| is probably good to keep as
a separate temp; |isRightMouseMenu| I would perhaps inline, but up to you.

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:121: value->SetBoolean(
Nit: Add a comment as to why we bother to communicate this value.

[frederic.jacob.78, 2015-06-24 21:35:03]

I finally reformat the if in the sendEvent function following pkasting comments.
For the rest I added the comments and blank lines as requested. Sorry for the
two cl, I send the first one when I received the last comments!

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:492: const base::CommandLine*
command_line =
On 2015/06/24 18:36:12, Robert Sesek wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:492: const base::CommandLine*
command_line =
On 2015/06/24 18:36:12, Robert Sesek wrote:
> naming: commandLine

Acknowledged.

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:494: const bool kioskMode =
command_line->HasSwitch(switches::kKioskMode);
On 2015/06/24 20:40:56, Peter Kasting wrote:
> Nit: Personally I think the explosion of temps here is a bit excessive.  I
would
> at least inline |command_line| into |kioskMode|, and I would probably inline
> that into the conditional below.  |isLeftMouseMenu| is probably good to keep
as
> a separate temp; |isRightMouseMenu| I would perhaps inline, but up to you.

I finally changed the code to only use the isLeftMouseMenu temp and embedded all
the rest in the if.

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:499: if (!kioskMode ||
(!isRightMouseMenu && !isLeftMouseMenu)) {
On 2015/06/24 18:36:12, Robert Sesek wrote:
> Leave a comment as to why this is here.

Done.

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/1091173005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:121: value->SetBoolean(
On 2015/06/24 20:40:56, Peter Kasting wrote:
> Nit: Add a comment as to why we bother to communicate this value.

Done.

[Peter Kasting, 2015-06-24 21:54:37]

Still LGTM

https://codereview.chromium.org/1091173005/diff/80001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/80001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:496: // swallow the events when
in kiosk mode to prevent context menu.
Nit: How about:

In kiosk mode, we want to prevent context menus from appearing, so simply
discard menu-generating events instead of passing them along.

https://codereview.chromium.org/1091173005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/1091173005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:121: // Communicate the
kiosk flag so we can disable showing the context menu
Nit: we -> the apps page?

[frederic.jacob.78, 2015-06-24 22:34:29]

I changed the comments as suggested.

https://codereview.chromium.org/1091173005/diff/80001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/80001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_application_mac.mm:496: // swallow the events when
in kiosk mode to prevent context menu.
On 2015/06/24 21:54:37, Peter Kasting wrote:
> Nit: How about:
> 
> In kiosk mode, we want to prevent context menus from appearing, so simply
> discard menu-generating events instead of passing them along.

No problem with that. I am speaking French, so I nearly ever argue over change
in wording ;)

https://codereview.chromium.org/1091173005/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/1091173005/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:121: // Communicate the
kiosk flag so we can disable showing the context menu
On 2015/06/24 21:54:37, Peter Kasting wrote:
> Nit: we -> the apps page?

Done.

[Robert Sesek, 2015-06-24 22:47:23]

LGTM

[Peter Kasting, 2015-06-24 23:10:37]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2015-06-24 23:10:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1091173005/#ps100001
(title: "Changed the comments")

[commit-bot: I haz the power, 2015-06-24 23:12:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091173005/100001

[commit-bot: I haz the power, 2015-06-24 23:23:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 23:23:27]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Kasting, 2015-06-24 23:24:18]

Looks like you need to get an OWNER approval for
chrome/browser/resources/ntp4/apps_page.js.

[frederic.jacob.78, 2015-06-24 23:55:16]

frederic.jacob.78@gmail.com changed reviewers:
+ dbeam@chromium.org

[frederic.jacob.78, 2015-06-25 00:01:39]

On 2015/06/24 23:24:18, Peter Kasting wrote:
> Looks like you need to get an OWNER approval for
> chrome/browser/resources/ntp4/apps_page.js.

I am adding dbeam as the reviewer for the apps_page.js. It was given to me by
git cl owners.

[Dan Beam, 2015-06-25 00:36:15]

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:493: const bool isLeftMouseMenu
= (event.type == NSLeftMouseDown) &&
nit: event.type == NSLeftMouseDown &&

(no need for extra parens)

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:500: ((event.type !=
NSRightMouseDown) &&
same: extra parens

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/ntp4/apps_page.js:282: if (!this.appData_.kioskMode) {
please add this to the @typedef AppInfo in
chrome/browser/resources/ntp4/page_list_view.js

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:118: // Please update it
whenever you add or remove any keys here.
update it

[frederic.jacob.78, 2015-06-25 22:25:34]

I added the @typedef AppInfo for the kioskMode variable and removed the extra
parens.

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:493: const bool isLeftMouseMenu
= (event.type == NSLeftMouseDown) &&
On 2015/06/25 00:36:15, Dan Beam wrote:
> nit: event.type == NSLeftMouseDown &&
> 
> (no need for extra parens)

Done.

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:500: ((event.type !=
NSRightMouseDown) &&
On 2015/06/25 00:36:15, Dan Beam wrote:
> same: extra parens

Done.

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/ntp4/apps_page.js (right):

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/resourc...
chrome/browser/resources/ntp4/apps_page.js:282: if (!this.appData_.kioskMode) {
On 2015/06/25 00:36:15, Dan Beam wrote:
> please add this to the @typedef AppInfo in
> chrome/browser/resources/ntp4/page_list_view.js

Done.

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

https://codereview.chromium.org/1091173005/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:118: // Please update it
whenever you add or remove any keys here.
On 2015/06/25 00:36:15, Dan Beam wrote:
> update it

Done.

[Dan Beam, 2015-06-25 23:18:23]

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:477: case NSLeftMouseDown:
I think you should be left mouse down specific code... in this left mouse
specific place :)

[frederic.jacob.78, 2015-06-26 00:24:22]

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:477: case NSLeftMouseDown:
On 2015/06/25 23:18:23, Dan Beam wrote:
> I think you should be left mouse down specific code... in this left mouse
> specific place :)

That was my first idea to create a new group with the left and right switch case
and break when we are in kiosk mode. But I though that we still want to keep the
tracking code in all condition. So this mean that the event handling code will
be duplicated and I still need to add an if for this new switch case block.
Don't you think that it is better to only add a bigger if for the current block
and not have to duplicate code? I know that it beat the purpose of the switch
case but I think that it is better to only add an if and not duplicate the event
handling code!

[Dan Beam, 2015-06-26 00:57:38]

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:477: case NSLeftMouseDown:
On 2015/06/26 00:24:22, frederic.jacob.78 wrote:
> On 2015/06/25 23:18:23, Dan Beam wrote:
> > I think you should be left mouse down specific code... in this left mouse
> > specific place :)
> 
> That was my first idea to create a new group with the left and right switch
case
> and break when we are in kiosk mode. But I though that we still want to keep
the
> tracking code in all condition. So this mean that the event handling code will
> be duplicated and I still need to add an if for this new switch case block.
> Don't you think that it is better to only add a bigger if for the current
block
> and not have to duplicate code? I know that it beat the purpose of the switch
> case but I think that it is better to only add an if and not duplicate the
event
> handling code!

I guess using fall-through is tricky and we should probably avoid it.  I
imagined this particular codepath was used a lot, so it should do less work. 
regardless, I think your code could be a bit easier to read.

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:492: 
I think this could be either at the beginning of this method or before the
tracked_objects::ScopedTracker initialization

auto type = event.type;
bool ctrlDown = [event modifierFlags] & NSControlKeyMask;
if (type == NSRightMouseDown || (type == NSLeftMouseDown && ctrlDown)) {
  // Events that would trigger a context menu.
  auto* command_line = base::CommandLine::ForCurrentProcess();
  if (command_line->HasSwitch(switches::kKioskMode))
    return;  // or break; if within this switch()?
}

[frederic.jacob.78, 2015-06-28 21:25:50]

I must admit that I am not super comfortable changing this code because it was
already approved by rsesek and he would need to re-approve it! But beside some
concern I like what is suggested, so I would like to pkasting and rsesek opinion
on this before changing the chrome_browser_application_mac.mm code.

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:492: 
On 2015/06/26 00:57:38, Dan Beam wrote:
> I think this could be either at the beginning of this method or before the
> tracked_objects::ScopedTracker initialization
> 
> auto type = event.type;
> bool ctrlDown = [event modifierFlags] & NSControlKeyMask;
> if (type == NSRightMouseDown || (type == NSLeftMouseDown && ctrlDown)) {
>   // Events that would trigger a context menu.
>   auto* command_line = base::CommandLine::ForCurrentProcess();
>   if (command_line->HasSwitch(switches::kKioskMode))
>     return;  // or break; if within this switch()?
> }

I like that but I have some concern about how I was told to handle this code.
First I assumed that we want to keep the tracker executing even when we discard
the event in kiosk mode, so I think that this code should be after it. Secondly,
I was told to minimize the number of local variables and that's why there is
only one for the left ctrl case, so I think that it should embed it in the test.


Beside that I like the way you suggest handling the ctrl key and that we only
test for the kiosk flag on right and left mouse down to minimize the number of
time this code is called. Also as it break on the special condition this do not
branch on normal case, which is normally faster.

[Peter Kasting, 2015-06-28 22:44:52]

Sorry, I wasn't paying enough attention to realize there was an assumption made
that I know enough to speak to.

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:477: case NSLeftMouseDown:
On 2015/06/26 00:24:22, frederic.jacob.78 wrote:
> On 2015/06/25 23:18:23, Dan Beam wrote:
> > I think you should be left mouse down specific code... in this left mouse
> > specific place :)
> 
> That was my first idea to create a new group with the left and right switch
case
> and break when we are in kiosk mode. But I though that we still want to keep
the
> tracking code in all condition.

No.  You should ensure the ScopedTracker is _not_ created when you're going to
eat the event, as we want it to only measure the time for the sendEvent call.

Given this, I would do this:

  switch (event.type) {
    case NSLeftMouseDown:
    case NSRightMouseDown:
      // In kiosk mode, ...
      bool kioskMode = base::CommandLine::ForCurrentProcess()->HasSwitch(
          switches::kKioskMode);
      bool ctrlDown = [event modifierFlags] & NSControlKeyMask;
      if (kioskMode && (event.type == NSRightMouseDown || ctrlDown))
        break;
      // FALL THROUGH
    case NSLeftMouseUp:
    case ...

Using temporary variables in this case as done above is fine since the code
doesn't get any longer or noticeably less efficient (and, really, in this case
efficiency differences don't matter), and naming things can make their function
more obvious.

[frederic.jacob.78, 2015-06-29 00:09:50]

On 2015/06/28 22:44:52, Peter Kasting wrote:
> Sorry, I wasn't paying enough attention to realize there was an assumption
made
> that I know enough to speak to.
> 
>
https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
> File chrome/browser/chrome_browser_application_mac.mm (right):
> 
>
https://codereview.chromium.org/1091173005/diff/120001/chrome/browser/chrome_...
> chrome/browser/chrome_browser_application_mac.mm:477: case NSLeftMouseDown:
> On 2015/06/26 00:24:22, frederic.jacob.78 wrote:
> > On 2015/06/25 23:18:23, Dan Beam wrote:
> > > I think you should be left mouse down specific code... in this left mouse
> > > specific place :)
> > 
> > That was my first idea to create a new group with the left and right switch
> case
> > and break when we are in kiosk mode. But I though that we still want to keep
> the
> > tracking code in all condition.
> 
> No.  You should ensure the ScopedTracker is _not_ created when you're going to
> eat the event, as we want it to only measure the time for the sendEvent call.
> 
> Given this, I would do this:
> 
>   switch (event.type) {
>     case NSLeftMouseDown:
>     case NSRightMouseDown:
>       // In kiosk mode, ...
>       bool kioskMode = base::CommandLine::ForCurrentProcess()->HasSwitch(
>           switches::kKioskMode);
>       bool ctrlDown = [event modifierFlags] & NSControlKeyMask;
>       if (kioskMode && (event.type == NSRightMouseDown || ctrlDown))
>         break;
>       // FALL THROUGH
>     case NSLeftMouseUp:
>     case ...
> 
> Using temporary variables in this case as done above is fine since the code
> doesn't get any longer or noticeably less efficient (and, really, in this case
> efficiency differences don't matter), and naming things can make their
function
> more obvious.

No problem! I think that was what Dan suggested at first but I did not
understand it.
I thought about it Friday when I was returning from work and I thought it was a
good idea
but as he suggested something else since, I did not reply to it. 

But my main concern here was that Dan suggested to change code that Robert
already
approved. So is changing code that already received LGTM by the owner be changed
after by 
request from another reviewer is something acceptable?

I would wait reply from Dan and Robert on what you are suggesting and I would
send a new
CL based on what they think of it.

[Peter Kasting, 2015-06-29 05:36:13]

On 2015/06/29 00:09:50, frederic.jacob.78 wrote:
> But my main concern here was that Dan suggested to change code that Robert
> already
> approved. So is changing code that already received LGTM by the owner be
changed
> after by 
> request from another reviewer is something acceptable?

It's a judgment call; if you think it's plausible that the other reviewer would
object, it doesn't hurt to ask for explicit re-approval.  That said, some people
stop paying attention to review threads once they've signed off, so sometimes
asking this will appear to get no response and you'll need to manually ping the
reviewer eventually.

For a case like this I think the sorts of changes being suggested are totally
reasonable and, if I were writing the change, I'd be comfortable just making
them and moving ahead with the landing, in the interests of not bugging people
more than is needed.  So both routes are reasonable IMO.

[Robert Sesek, 2015-06-29 15:12:17]

I'm fine with the code as-written.

[Dan Beam, 2015-07-06 21:44:00]

chrome/browser/resources and chrome/browser/ui/webui lgtm but I'd still rather
do less work in chrome_browser_application_mac.mm

[frederic.jacob.78, 2015-07-07 10:52:59]

Here the change.

[Robert Sesek, 2015-07-07 15:38:36]

https://codereview.chromium.org/1091173005/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:484: if (kioskMode &&
(event.type == NSRightMouseDown || ctrlDown))
You should be consistent with accessor dot notation and method invocation
syntax. This should be either [event type] or the above should be
event.modifierFlags.

[frederic.jacob.78, 2015-07-07 22:48:48]

I changed the code to use the [] notation.

https://codereview.chromium.org/1091173005/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:484: if (kioskMode &&
(event.type == NSRightMouseDown || ctrlDown))
On 2015/07/07 15:38:35, Robert Sesek wrote:
> You should be consistent with accessor dot notation and method invocation
> syntax. This should be either [event type] or the above should be
> event.modifierFlags.

Done.

[Robert Sesek, 2015-07-07 22:49:35]

LGTM

[Peter Kasting, 2015-07-07 23:02:50]

lgtm

https://codereview.chromium.org/1091173005/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:487: 
Please put an explicit "// FALL THROUGH" comment here.

[frederic.jacob.78, 2015-07-07 23:10:56]

https://codereview.chromium.org/1091173005/diff/160001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_application_mac.mm (right):

https://codereview.chromium.org/1091173005/diff/160001/chrome/browser/chrome_...
chrome/browser/chrome_browser_application_mac.mm:487: 
On 2015/07/07 23:02:49, Peter Kasting wrote:
> Please put an explicit "// FALL THROUGH" comment here.

Done.

[Peter Kasting, 2015-07-07 23:14:42]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2015-07-07 23:14:43]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
rsesek@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1091173005/#ps180001
(title: "Add the FALL THROUGH comment")

[Peter Kasting, 2015-07-07 23:14:57]

The CQ bit was unchecked by pkasting@chromium.org

[Peter Kasting, 2015-07-07 23:15:14]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-07-07 23:17:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091173005/180001

[commit-bot: I haz the power, 2015-07-08 01:11:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-08 01:11:54]

Try jobs failed on following builders:
  linux_arm_compile on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

(exceeded global retry quota)

[Peter Kasting, 2015-07-08 01:16:07]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-07-08 01:19:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1091173005/180001

[commit-bot: I haz the power, 2015-07-08 02:18:17]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-07-08 02:19:08]

Patchset 10 (id:??) landed as
https://crrev.com/25e2a6ce5742821aedf91746804487ae64ce2190
Cr-Commit-Position: refs/heads/master@{#337741}