Bug fix for the pointer lock string problem

Bug fix for the pointer lock string problem, including:
(1) make the tile of the denyButton adaptive, such that
 "Deny" will show on a mouse lock request instead of "Exit full screen."
(2) add unittest to verify its behavior.

XIB changes:
* Remove the default deny button title IDS_FULLSCREEN_EXIT_FULLSCREEN.

BUG=139944
Committed: https://crrev.com/70091e75f4af58bff364d591269bc3b465e9b4d8
Cr-Commit-Position: refs/heads/master@{#313638}

[Jialiu Lin, 2015-01-21 18:21:31]

jialiul@chromium.org changed reviewers:
+ felt@chromium.org

[Jialiu Lin, 2015-01-21 18:21:32]

Bug fix for the pointer lock string problem, including:
(1) switch positions of Allow and Deny button to make it the same as
 Chromes on other platforms.
(2) make the tile of the denyButton adaptive, such that
 "Deny" will show on a mouse lock request instead of "Exit full screen."
(3) add unittest to verify its behavior.

p.s. I run all the unittests locally, seems fine, however there are two try
failures (which I have hard time to understand the try failure output.)

[chromium-reviews, 2015-01-22 01:08:00]

Hi Adrienne,
I tried my patch locally, it works fine with
http://scheib.github.com/HTMLMisc/PointerLockAndFullscreen.html. After
uploading this CL, the mac_chromium_rel_ng try server keeps failing on a
couple of unit tests (
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...).
But strangely, my changes can pass all unit tests locally.  Did I miss
anything? I find myself having a hard time interpreting this try bot
result. Could you give me some hint?

Thanks,
Jialiu

On Wed, Jan 21, 2015 at 10:21 AM, <jialiul@chromium.org> wrote:

> Reviewers: felt,
>
> Message:
> Bug fix for the pointer lock string problem, including:
> (1) switch positions of Allow and Deny button to make it the same as
>  Chromes on other platforms.
> (2) make the tile of the denyButton adaptive, such that
>  "Deny" will show on a mouse lock request instead of "Exit full screen."
> (3) add unittest to verify its behavior.
>
> p.s. I run all the unittests locally, seems fine, however there are two try
> failures (which I have hard time to understand the try failure output.)
>
> Description:
> Bug fix for the pointer lock string problem, including:
> (1) switch positions of Allow and Deny button to make it the same as
>  Chromes on other platforms.
> (2) make the tile of the denyButton adaptive, such that
>  "Deny" will show on a mouse lock request instead of "Exit full screen."
> (3) add unittest to verify its behavior.
>
> BUG=139944
>
> Please review this at https://codereview.chromium.org/856253002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+41, -2 lines):
>   M chrome/app/nibs/ExclusiveAccessBubble.xib
>   M chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
>   M chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller_unittest.mm
>
>
> Index: chrome/app/nibs/ExclusiveAccessBubble.xib
> diff --git a/chrome/app/nibs/ExclusiveAccessBubble.xib b/chrome/app/nibs/
> ExclusiveAccessBubble.xib
> index cb646222305a0568a3ec49bf4322c659c2e504a2..
> 1a294788eef5ef4ade9f6311ff6189c147744acc 100644
> --- a/chrome/app/nibs/ExclusiveAccessBubble.xib
> +++ b/chrome/app/nibs/ExclusiveAccessBubble.xib
> @@ -78,7 +78,7 @@
>
>                       <object class="NSButton" id="436635720">
>
>                               <reference key="NSNextResponder"
> ref="1031271207"/>
>
>                               <int key="NSvFlags">268</int>
> -
>                              <string key="NSFrame">{{27, 1}, {125,
> 32}}</string>
> +
>                              <string key="NSFrame">{{102, 1}, {125,
> 32}}</string>
>
>                               <reference key="NSSuperview"
> ref="1031271207"/>
>
>                               <reference key="NSWindow"/>
>
>                               <bool key="NSEnabled">YES</bool>
> @@ -104,7 +104,7 @@
>
>                       <object class="NSButton" id="292228158">
>
>                               <reference key="NSNextResponder"
> ref="1031271207"/>
>
>                               <int key="NSvFlags">268</int>
> -
>                              <string key="NSFrame">{{152, 1}, {75,
> 32}}</string>
> +
>                              <string key="NSFrame">{{27, 1}, {75,
> 32}}</string>
>
>                               <reference key="NSSuperview"
> ref="1031271207"/>
>
>                               <reference key="NSWindow"/>
>
>                               <bool key="NSEnabled">YES</bool>
> Index: chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller.mm
> diff --git a/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller.mm b/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller.mm
> index d6c0de1689d19206affeb70769be10d861a2380a..
> c1c3a0a21cdb4b0e984ab36d4af79a0a0373ada5 100644
> --- a/chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
> +++ b/chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
> @@ -258,6 +258,12 @@ const float kHideDuration = 0.7;
>    labelFrame.origin.x += NSWidth(labelFrame) - NSWidth(textFrame);
>    labelFrame.size = textFrame.size;
>    [exitLabel_ setFrame:labelFrame];
> +
> +  // Update the title of denyButton_ according to the current bubbleType_
> +  NSString* denyButtonText =
> +      SysUTF16ToNSString(
> +        exclusive_access_bubble::GetDenyButtonTextForType(bubbleType_));
> +  [denyButton_ setTitle: denyButtonText];
>  }
>
>  - (NSString*)getLabelText {
> Index: chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller_unittest.mm
> diff --git a/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller_unittest.mm b/chrome/browser/ui/cocoa/excl
> usive_access_bubble_window_controller_unittest.mm
> index 1d062ef73ae4337ec945f2cd58446ee0bb2fbfb6..
> 013407a4034c70d29babe52f1d19cc2ed2ce74f9 100644
> --- a/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller_unittest.mm
> +++ b/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> controller_unittest.mm
> @@ -10,6 +10,7 @@
>  #include "chrome/browser/ui/cocoa/browser_window_controller.h"
>  #include "chrome/browser/ui/cocoa/cocoa_profile_test.h"
>  #include "chrome/browser/ui/tabs/tab_strip_model.h"
> +#include "chrome/grit/generated_resources.h"
>  #include "chrome/test/base/testing_profile.h"
>  #include "content/public/browser/notification_service.h"
>  #include "content/public/browser/site_instance.h"
> @@ -17,6 +18,8 @@
>  #include "content/public/test/test_utils.h"
>  #include "testing/gtest_mac.h"
>  #include "ui/base/accelerators/platform_accelerator_cocoa.h"
> +#include "ui/base/l10n/l10n_util.h"
> +#include "ui/base/l10n/l10n_util_mac.h"
>
>  using content::SiteInstance;
>  using content::WebContents;
> @@ -26,11 +29,13 @@ using content::WebContents;
>  + (NSString*)keyCommandString;
>  + (NSString*)keyCombinationForAccelerator:
>          (const ui::PlatformAcceleratorCocoa&)item;
> +- (void)initializeLabel;
>  @end
>
>  @interface ExclusiveAccessBubbleWindowController (ExposedForTesting)
>  - (NSTextField*)exitLabelPlaceholder;
>  - (NSTextView*)exitLabel;
> +- (NSString*)denyButtonText;
>  @end
>
>  @implementation ExclusiveAccessBubbleWindowController (ExposedForTesting)
> @@ -41,6 +46,10 @@ using content::WebContents;
>  - (NSTextView*)exitLabel {
>    return exitLabel_;
>  }
> +
> +- (NSString*)denyButtonText {
> +  return [denyButton_ title];
> +}
>  @end
>
>  class ExclusiveAccessBubbleWindowControllerTest : public
> CocoaProfileTest {
> @@ -129,3 +138,27 @@ TEST_F(ExclusiveAccessBubbleWindowControllerTest,
> ShortcutText) {
>    EXPECT_NSEQ(cmd_shift_f_text, cmd_F_text);
>    EXPECT_NSEQ(@"\u2318\u21E7F", cmd_shift_f_text);
>  }
> +
> +// http://crbug.com/139944
> +TEST_F(ExclusiveAccessBubbleWindowControllerTest, DenyButtonText) {
> +  controller_.reset([[ExclusiveAccessBubbleWindowController alloc]
> +      initWithOwner:nil
> +      browser:browser()
> +      url:GURL()
> +      bubbleType:EXCLUSIVE_ACCESS_BUBBLE_TYPE_MOUSELOCK_BUTTONS]);
> +  [controller_ initializeLabel];
> +  NSString* mouselock_deny_button_text = [controller_ denyButtonText];
> +  EXPECT_NSEQ(l10n_util::GetNSString(IDS_FULLSCREEN_DENY),
> +    mouselock_deny_button_text);
> +
> +  controller_.reset([[ExclusiveAccessBubbleWindowController alloc]
> +      initWithOwner:nil
> +      browser:browser()
> +      url:GURL()
> +      bubbleType:EXCLUSIVE_ACCESS_BUBBLE_TYPE_FULLSCREEN_
> MOUSELOCK_BUTTONS]);
> +  [controller_ initializeLabel];
> +  NSString* fullscreen_mouselock_deny_button_text =
> +      [controller_ denyButtonText];
> +  EXPECT_NSEQ(l10n_util::GetNSString(IDS_FULLSCREEN_EXIT),
> +    fullscreen_mouselock_deny_button_text);
> +}
>
>
>


-- 
Jialiu Lin |  Privacy Analysis Engineer  |  jialiul@google.com

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

[felt, 2015-01-22 23:41:28]

On 2015/01/22 01:08:00, chromium-reviews wrote:
> Hi Adrienne,
> I tried my patch locally, it works fine with
> http://scheib.github.com/HTMLMisc/PointerLockAndFullscreen.html. After
> uploading this CL, the mac_chromium_rel_ng try server keeps failing on a
> couple of unit tests (
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...).
> But strangely, my changes can pass all unit tests locally.  Did I miss
> anything? I find myself having a hard time interpreting this try bot
> result. Could you give me some hint?
> 
> Thanks,
> Jialiu
> 
> On Wed, Jan 21, 2015 at 10:21 AM, <mailto:jialiul@chromium.org> wrote:
> 
> > Reviewers: felt,
> >
> > Message:
> > Bug fix for the pointer lock string problem, including:
> > (1) switch positions of Allow and Deny button to make it the same as
> >  Chromes on other platforms.
> > (2) make the tile of the denyButton adaptive, such that
> >  "Deny" will show on a mouse lock request instead of "Exit full screen."
> > (3) add unittest to verify its behavior.
> >
> > p.s. I run all the unittests locally, seems fine, however there are two try
> > failures (which I have hard time to understand the try failure output.)
> >
> > Description:
> > Bug fix for the pointer lock string problem, including:
> > (1) switch positions of Allow and Deny button to make it the same as
> >  Chromes on other platforms.
> > (2) make the tile of the denyButton adaptive, such that
> >  "Deny" will show on a mouse lock request instead of "Exit full screen."
> > (3) add unittest to verify its behavior.
> >
> > BUG=139944
> >
> > Please review this at https://codereview.chromium.org/856253002/
> >
> > Base URL: https://chromium.googlesource.com/chromium/src.git@master
> >
> > Affected files (+41, -2 lines):
> >   M chrome/app/nibs/ExclusiveAccessBubble.xib
> >   M chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
> >   M chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> > controller_unittest.mm
> >
> >
> > Index: chrome/app/nibs/ExclusiveAccessBubble.xib
> > diff --git a/chrome/app/nibs/ExclusiveAccessBubble.xib b/chrome/app/nibs/
> > ExclusiveAccessBubble.xib
> > index cb646222305a0568a3ec49bf4322c659c2e504a2..
> > 1a294788eef5ef4ade9f6311ff6189c147744acc 100644
> > --- a/chrome/app/nibs/ExclusiveAccessBubble.xib
> > +++ b/chrome/app/nibs/ExclusiveAccessBubble.xib
> > @@ -78,7 +78,7 @@
> >
> >                       <object class="NSButton" id="436635720">
> >
> >                               <reference key="NSNextResponder"
> > ref="1031271207"/>
> >
> >                               <int key="NSvFlags">268</int>
> > -
> >                              <string key="NSFrame">{{27, 1}, {125,
> > 32}}</string>
> > +
> >                              <string key="NSFrame">{{102, 1}, {125,
> > 32}}</string>
> >
> >                               <reference key="NSSuperview"
> > ref="1031271207"/>
> >
> >                               <reference key="NSWindow"/>
> >
> >                               <bool key="NSEnabled">YES</bool>
> > @@ -104,7 +104,7 @@
> >
> >                       <object class="NSButton" id="292228158">
> >
> >                               <reference key="NSNextResponder"
> > ref="1031271207"/>
> >
> >                               <int key="NSvFlags">268</int>
> > -
> >                              <string key="NSFrame">{{152, 1}, {75,
> > 32}}</string>
> > +
> >                              <string key="NSFrame">{{27, 1}, {75,
> > 32}}</string>
> >
> >                               <reference key="NSSuperview"
> > ref="1031271207"/>
> >
> >                               <reference key="NSWindow"/>
> >
> >                               <bool key="NSEnabled">YES</bool>
> > Index: chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> > controller.mm
> > diff --git a/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> > controller.mm b/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> > controller.mm
> > index d6c0de1689d19206affeb70769be10d861a2380a..
> > c1c3a0a21cdb4b0e984ab36d4af79a0a0373ada5 100644
> > --- a/chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
> > +++ b/chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
> > @@ -258,6 +258,12 @@ const float kHideDuration = 0.7;
> >    labelFrame.origin.x += NSWidth(labelFrame) - NSWidth(textFrame);
> >    labelFrame.size = textFrame.size;
> >    [exitLabel_ setFrame:labelFrame];
> > +
> > +  // Update the title of denyButton_ according to the current bubbleType_
> > +  NSString* denyButtonText =
> > +      SysUTF16ToNSString(
> > +        exclusive_access_bubble::GetDenyButtonTextForType(bubbleType_));
> > +  [denyButton_ setTitle: denyButtonText];
> >  }
> >
> >  - (NSString*)getLabelText {
> > Index: chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> > controller_unittest.mm
> > diff --git a/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> > controller_unittest.mm b/chrome/browser/ui/cocoa/excl
> > usive_access_bubble_window_controller_unittest.mm
> > index 1d062ef73ae4337ec945f2cd58446ee0bb2fbfb6..
> > 013407a4034c70d29babe52f1d19cc2ed2ce74f9 100644
> > --- a/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> > controller_unittest.mm
> > +++ b/chrome/browser/ui/cocoa/exclusive_access_bubble_window_
> > controller_unittest.mm
> > @@ -10,6 +10,7 @@
> >  #include "chrome/browser/ui/cocoa/browser_window_controller.h"
> >  #include "chrome/browser/ui/cocoa/cocoa_profile_test.h"
> >  #include "chrome/browser/ui/tabs/tab_strip_model.h"
> > +#include "chrome/grit/generated_resources.h"
> >  #include "chrome/test/base/testing_profile.h"
> >  #include "content/public/browser/notification_service.h"
> >  #include "content/public/browser/site_instance.h"
> > @@ -17,6 +18,8 @@
> >  #include "content/public/test/test_utils.h"
> >  #include "testing/gtest_mac.h"
> >  #include "ui/base/accelerators/platform_accelerator_cocoa.h"
> > +#include "ui/base/l10n/l10n_util.h"
> > +#include "ui/base/l10n/l10n_util_mac.h"
> >
> >  using content::SiteInstance;
> >  using content::WebContents;
> > @@ -26,11 +29,13 @@ using content::WebContents;
> >  + (NSString*)keyCommandString;
> >  + (NSString*)keyCombinationForAccelerator:
> >          (const ui::PlatformAcceleratorCocoa&)item;
> > +- (void)initializeLabel;
> >  @end
> >
> >  @interface ExclusiveAccessBubbleWindowController (ExposedForTesting)
> >  - (NSTextField*)exitLabelPlaceholder;
> >  - (NSTextView*)exitLabel;
> > +- (NSString*)denyButtonText;
> >  @end
> >
> >  @implementation ExclusiveAccessBubbleWindowController (ExposedForTesting)
> > @@ -41,6 +46,10 @@ using content::WebContents;
> >  - (NSTextView*)exitLabel {
> >    return exitLabel_;
> >  }
> > +
> > +- (NSString*)denyButtonText {
> > +  return [denyButton_ title];
> > +}
> >  @end
> >
> >  class ExclusiveAccessBubbleWindowControllerTest : public
> > CocoaProfileTest {
> > @@ -129,3 +138,27 @@ TEST_F(ExclusiveAccessBubbleWindowControllerTest,
> > ShortcutText) {
> >    EXPECT_NSEQ(cmd_shift_f_text, cmd_F_text);
> >    EXPECT_NSEQ(@"\u2318\u21E7F", cmd_shift_f_text);
> >  }
> > +
> > +// http://crbug.com/139944
> > +TEST_F(ExclusiveAccessBubbleWindowControllerTest, DenyButtonText) {
> > +  controller_.reset([[ExclusiveAccessBubbleWindowController alloc]
> > +      initWithOwner:nil
> > +      browser:browser()
> > +      url:GURL()
> > +      bubbleType:EXCLUSIVE_ACCESS_BUBBLE_TYPE_MOUSELOCK_BUTTONS]);
> > +  [controller_ initializeLabel];
> > +  NSString* mouselock_deny_button_text = [controller_ denyButtonText];
> > +  EXPECT_NSEQ(l10n_util::GetNSString(IDS_FULLSCREEN_DENY),
> > +    mouselock_deny_button_text);
> > +
> > +  controller_.reset([[ExclusiveAccessBubbleWindowController alloc]
> > +      initWithOwner:nil
> > +      browser:browser()
> > +      url:GURL()
> > +      bubbleType:EXCLUSIVE_ACCESS_BUBBLE_TYPE_FULLSCREEN_
> > MOUSELOCK_BUTTONS]);
> > +  [controller_ initializeLabel];
> > +  NSString* fullscreen_mouselock_deny_button_text =
> > +      [controller_ denyButtonText];
> > +  EXPECT_NSEQ(l10n_util::GetNSString(IDS_FULLSCREEN_EXIT),
> > +    fullscreen_mouselock_deny_button_text);
> > +}
> >
> >
> >
> 
> 
> -- 
> Jialiu Lin |  Privacy Analysis Engineer  |  mailto:jialiul@google.com
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Just to check: are you building a Debug build, or a Release build for testing?

[felt, 2015-01-22 23:46:52]

https://codereview.chromium.org/856253002/diff/1/chrome/browser/ui/cocoa/excl...
File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
(right):

https://codereview.chromium.org/856253002/diff/1/chrome/browser/ui/cocoa/excl...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:265:
exclusive_access_bubble::GetDenyButtonTextForType(bubbleType_));
It looks like this is providing a bubbleType_ on that one builder that isn't
handled by the GetDenyButtonTextForType() switch statement. I don't know why it
won't reproduce locally, but my suggestion would be to add some debugging text
to print out the value of bubbleType_ and then run the one bot that's giving you
trouble. That will hopefully let you see what this extra value is and why it
isn't being handled in GetDenyButtonTextForType().

[Jialiu Lin, 2015-01-23 18:35:31]

On 2015/01/22 23:46:52, felt wrote:
>
https://codereview.chromium.org/856253002/diff/1/chrome/browser/ui/cocoa/excl...
> File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
> (right):
> 
>
https://codereview.chromium.org/856253002/diff/1/chrome/browser/ui/cocoa/excl...
> chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:265:
> exclusive_access_bubble::GetDenyButtonTextForType(bubbleType_));
> It looks like this is providing a bubbleType_ on that one builder that isn't
> handled by the GetDenyButtonTextForType() switch statement. I don't know why
it
> won't reproduce locally, but my suggestion would be to add some debugging text
> to print out the value of bubbleType_ and then run the one bot that's giving
you
> trouble. That will hopefully let you see what this extra value is and why it
> isn't being handled in GetDenyButtonTextForType().

Thanks, Adrienne! I added conditional check for the types that handled by
GetDenyButtonTextForType(), Problem fixed.

[Jialiu Lin, 2015-01-23 22:55:47]

https://codereview.chromium.org/856253002/diff/1/chrome/browser/ui/cocoa/excl...
File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
(right):

https://codereview.chromium.org/856253002/diff/1/chrome/browser/ui/cocoa/excl...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:265:
exclusive_access_bubble::GetDenyButtonTextForType(bubbleType_));
On 2015/01/22 23:46:52, felt wrote:
> It looks like this is providing a bubbleType_ on that one builder that isn't
> handled by the GetDenyButtonTextForType() switch statement. I don't know why
it
> won't reproduce locally, but my suggestion would be to add some debugging text
> to print out the value of bubbleType_ and then run the one bot that's giving
you
> trouble. That will hopefully let you see what this extra value is and why it
> isn't being handled in GetDenyButtonTextForType().

Done.

[felt, 2015-01-24 08:28:52]

I'm not qualified to review mac code, but it's looking good -- please go ahead
and send to the proper OWNERS reviewer!

[Jialiu Lin, 2015-01-25 07:36:53]

jialiul@chromium.org changed reviewers:
+ mark@chromium.org
- felt@chromium.org

[Jialiu Lin, 2015-01-25 07:36:53]

Hi Mark,
Could you take a look at this CL? 

Thanks,
Jialiu

[Jialiu Lin, 2015-01-27 18:54:29]

On 2015/01/25 07:36:53, JialiuLin wrote:
> Hi Mark,
> Could you take a look at this CL? 
> 
> Thanks,
> Jialiu

A gentle ping~~

[Mark Mentovai, 2015-01-27 19:24:40]

On Mac, buttons for “confirm” or “proceed” actions are laid out to the right of
buttons for “cancel” actions. I don’t see anywhere that you’ve discussed moving
the button layout away from the established platform convention.

[Avi (use Gerrit), 2015-01-27 19:27:12]

random drive-by

https://codereview.chromium.org/856253002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
(right):

https://codereview.chromium.org/856253002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:270:
[denyButton_ setTitle: denyButtonText];
no space after :

[Jialiu Lin, 2015-01-27 19:48:06]

On 2015/01/27 19:24:40, Mark Mentovai wrote:
> On Mac, buttons for “confirm” or “proceed” actions are laid out to the
right of
> buttons for “cancel” actions. I don’t see anywhere that you’ve discussed
moving
> the button layout away from the established platform convention.

Hi Mark,
Thanks for your reply. Sorry I wasn't aware of this Mac button laid out
guideline.
I reverted this file chrome/app/nibs/ExclusiveAccessBubble.xib/. Now "Deny" is
on the left of "Allow".

[Jialiu Lin, 2015-01-27 19:51:08]

jialiul@chromium.org changed reviewers:
+ avi@chromium.org

[Jialiu Lin, 2015-01-27 19:51:09]

Thanks, Mark and Avi. 
The style problem is fixed.

https://codereview.chromium.org/856253002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
(right):

https://codereview.chromium.org/856253002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:270:
[denyButton_ setTitle: denyButtonText];
On 2015/01/27 19:27:12, Avi wrote:
> no space after :

Done.

[Mark Mentovai, 2015-01-27 20:15:13]

Update the CL description to, then.

[Jialiu Lin, 2015-01-27 20:39:03]

On 2015/01/27 20:15:13, Mark Mentovai wrote:
> Update the CL description to, then.

Oops, my bad. The CL description is updated. Thanks!

[Mark Mentovai, 2015-01-27 21:32:16]

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
(right):

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:47: // sets
|exitLabelPlaceholder_| to nil.
Revise comment.

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:263: if
(bubbleType_ == EXCLUSIVE_ACCESS_BUBBLE_TYPE_FULLSCREEN_BUTTONS ||
Call exclusive_access_bubble::ShowButtonsForType() instead of duplicating its
logic here.

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:269:
exclusive_access_bubble::GetDenyButtonTextForType(bubbleType_));
Since you’re now always populating the deny button text (if the deny button is
to be shown) right here, I think you should remove the to-be-localized text from
the nib. That will prevent the work from being done to get the default
IDS_FULLSCREEN_DENY label if it’s just going to be overwritten, and the
duplicate work from being done to get it twice if it’s in fact the correct
label. It will also provide a better visual indication of a problem (label-less
button?) in case this code doesn’t execute properly.

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:271: }
Perhaps the showButtons:NO call should be here in an else instead of in
-showWindow. Conceptually, I believe it makes more sense to do it here because
it’s a choice between “set proper button text” and “don’t show button at all”.
The ShowButtonsForType() check in -showWindow isn’t as much about showing or
hiding the buttons as it is about arranging to dismiss the window when no
buttons are present. Hiding the buttons is more related to this code than that
code.

With this change, I’m starting to think that this method could be named
something better than -initializeLabel, since it’s now doing more than just
initializing its label.

[Jialiu Lin, 2015-01-28 02:47:09]

Thanks, Mark. Your comments have been addressed.

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm
(right):

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:47: // sets
|exitLabelPlaceholder_| to nil.
On 2015/01/27 21:32:16, Mark Mentovai wrote:
> Revise comment.

Done.

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:263: if
(bubbleType_ == EXCLUSIVE_ACCESS_BUBBLE_TYPE_FULLSCREEN_BUTTONS ||
On 2015/01/27 21:32:16, Mark Mentovai wrote:
> Call exclusive_access_bubble::ShowButtonsForType() instead of duplicating its
> logic here.

Done.

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:269:
exclusive_access_bubble::GetDenyButtonTextForType(bubbleType_));
On 2015/01/27 21:32:16, Mark Mentovai wrote:
> Since you’re now always populating the deny button text (if the deny button is
> to be shown) right here, I think you should remove the to-be-localized text
from
> the nib. That will prevent the work from being done to get the default
> IDS_FULLSCREEN_DENY label if it’s just going to be overwritten, and the
> duplicate work from being done to get it twice if it’s in fact the correct
> label. It will also provide a better visual indication of a problem
(label-less
> button?) in case this code doesn’t execute properly.

Done.

https://codereview.chromium.org/856253002/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/exclusive_access_bubble_window_controller.mm:271: }
On 2015/01/27 21:32:16, Mark Mentovai wrote:
> Perhaps the showButtons:NO call should be here in an else instead of in
> -showWindow. Conceptually, I believe it makes more sense to do it here because
> it’s a choice between “set proper button text” and “don’t show button at
all”.
> The ShowButtonsForType() check in -showWindow isn’t as much about showing or
> hiding the buttons as it is about arranging to dismiss the window when no
> buttons are present. Hiding the buttons is more related to this code than that
> code.
> 
> With this change, I’m starting to think that this method could be named
> something better than -initializeLabel, since it’s now doing more than just
> initializing its label.

Added the else clause, and changed the function name to 
-initializeLabelAndButton

[Mark Mentovai, 2015-01-28 03:47:41]

LGTM. Update the description to say how you’ve changed the nib. (It’s obvious
this time, but nib changes should be accompanied by a description of the change
per http://www.chromium.org/developers/design-documents/mac-xib-files, because
in most cases it’s not easy to tell what’s changed by looking at the diff.)

[Jialiu Lin, 2015-01-28 20:41:11]

On 2015/01/28 03:47:41, Mark Mentovai wrote:
> LGTM. Update the description to say how you’ve changed the nib. (It’s obvious
> this time, but nib changes should be accompanied by a description of the
change
> per http://www.chromium.org/developers/design-documents/mac-xib-files, because
> in most cases it’s not easy to tell what’s changed by looking at the diff.)

Thanks for the pointer and reminder, Mark. Description of Xib change added.

[Mark Mentovai, 2015-01-28 20:47:02]

LGTM

[Jialiu Lin, 2015-01-28 20:48:23]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2015-01-28 20:48:29]

The CQ bit was unchecked by jialiul@chromium.org

[Jialiu Lin, 2015-01-28 20:48:34]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2015-01-28 20:48:45]

The CQ bit was unchecked by jialiul@chromium.org

[Jialiu Lin, 2015-01-29 00:19:38]

The CQ bit was checked by jialiul@chromium.org

[commit-bot: I haz the power, 2015-01-29 00:20:03]

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

[commit-bot: I haz the power, 2015-01-29 00:31:01]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-29 00:32:01]

Patchset 7 (id:??) landed as
https://crrev.com/70091e75f4af58bff364d591269bc3b465e9b4d8
Cr-Commit-Position: refs/heads/master@{#313638}