Excluded non-chromeos source files in chromeos build

Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For an example, see the build failures in https://codereview.chromium.org/2151513002/#ps100001.
Tested by applying the above patch and verifying that the build passes.

BUG=615893
Committed: https://crrev.com/1831391aeba23ff66f87678b619ddee6113066d2
Cr-Commit-Position: refs/heads/master@{#406443}

[Jane, 2016-07-15 14:53:52]

Description was changed from

==========
Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in
chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For
an example, see the build failures in
https://codereview.chromium.org/2151513002/

BUG=615893
==========

to

==========
Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in
chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For
an example, see the build failures in
https://codereview.chromium.org/2151513002/#ps100001

BUG=615893
==========

[Jane, 2016-07-15 14:56:10]

Description was changed from

==========
Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in
chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For
an example, see the build failures in
https://codereview.chromium.org/2151513002/#ps100001

BUG=615893
==========

to

==========
Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in
chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For
an example, see the build failures in
https://codereview.chromium.org/2151513002/#ps100001.
Tested by applying the above patch and verifying that the build passes.

BUG=615893
==========

[Jane, 2016-07-15 15:00:17]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[Jane, 2016-07-15 15:00:30]

janeliulwq@google.com changed reviewers:
+ msw@chromium.org, rogerta@chromium.org

[Jane, 2016-07-15 15:00:30]

janeliulwq@google.com changed required reviewers:
+ msw@chromium.org, rogerta@chromium.org

[Jane, 2016-07-15 15:00:31]

Thanks!

[commit-bot: I haz the power, 2016-07-15 15:00:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2016-07-15 15:51:26]

msw@chromium.org changed reviewers:
+ tapted@chromium.org

[msw, 2016-07-15 15:51:27]

Why do we have asterisks before our names in the reviewers field?

https://codereview.chromium.org/2148293003/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_dialogs_views.cc (right):

https://codereview.chromium.org/2148293003/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_dialogs_views.cc:12: #include
"chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h"
You'll need to only include this #if !defined(OS_CHROMEOS)

https://codereview.chromium.org/2148293003/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/2148293003/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:3071:
'browser/ui/views/profiles/profile_chooser_view.cc',
I wonder if we intend to support these in Mac Views, and if they could just move
to chrome_browser_ui_views_non_chromeos_sources; +tapted@ for thoughts.
(profile_chooser_view_browsertest.cc comment: # This should be brought up on OSX
Views but not CrOS.)

https://codereview.chromium.org/2148293003/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:3072:
'browser/ui/views/profiles/profile_chooser_view.h',
You'll need to make includes of this header non-cros-specific.

[tapted, 2016-07-18 21:20:59]

> Why do we have asterisks before our names in the reviewers field?

I seem to recall this being a code for "I'll wait for your lgtm, but maybe not
wait for an lgtm from those without asterisks before landing"

https://codereview.chromium.org/2148293003/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/2148293003/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:3071:
'browser/ui/views/profiles/profile_chooser_view.cc',
On 2016/07/15 15:51:27, msw wrote:
> I wonder if we intend to support these in Mac Views, and if they could just
move
> to chrome_browser_ui_views_non_chromeos_sources; +tapted@ for thoughts.
> (profile_chooser_view_browsertest.cc comment: # This should be brought up on
OSX
> Views but not CrOS.)

ooh - yep! chrome_browser_ui_views_non_chromeos_sources is the right place for
these. As it is, I think this will break the `compile` step with
mac_views_browser=1 in GYP_DEFINES

chrome_browser_ui_views_non_chromeos_sources is already guarded by a similar
condition, but with the fix for mac_views_browser=1

            ['chromeos == 0 and (OS!="mac" or mac_views_browser==1)', {
              'sources': [ '<@(chrome_browser_ui_views_non_chromeos_sources)' ],
            }],

[Jane, 2016-07-18 22:06:36]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 22:07:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 22:16:14]

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

[commit-bot: I haz the power, 2016-07-18 22:16:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Jane, 2016-07-18 22:47:09]

Patchset #3 (id:40001) has been deleted

[Jane, 2016-07-18 22:48:01]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 22:48:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 23:17:41]

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

[commit-bot: I haz the power, 2016-07-18 23:17:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jane, 2016-07-18 23:37:03]

Patchset #3 (id:60001) has been deleted

[Jane, 2016-07-18 23:38:00]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 23:39:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 23:55:13]

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

[commit-bot: I haz the power, 2016-07-18 23:55:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jane, 2016-07-19 02:17:50]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[Jane, 2016-07-19 02:17:56]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-07-19 02:18:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 02:32:16]

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

[commit-bot: I haz the power, 2016-07-19 02:32:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jane, 2016-07-19 02:37:24]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[Jane, 2016-07-19 02:37:28]

Patchset #4 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-07-19 02:38:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 03:53:19]

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

[commit-bot: I haz the power, 2016-07-19 03:53:21]

Dry run: This issue passed the CQ dry run.

[Jane, 2016-07-19 04:33:55]

PTAL & let me know if I missed anything, thanks!

https://codereview.chromium.org/2148293003/diff/1/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tab_dialogs_views.cc (right):

https://codereview.chromium.org/2148293003/diff/1/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tab_dialogs_views.cc:12: #include
"chrome/browser/ui/views/sync/profile_signin_confirmation_dialog_views.h"
On 2016/07/15 15:51:27, msw wrote:
> You'll need to only include this #if !defined(OS_CHROMEOS)

Done.

https://codereview.chromium.org/2148293003/diff/1/chrome/chrome_browser_ui.gypi
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/2148293003/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:3071:
'browser/ui/views/profiles/profile_chooser_view.cc',
On 2016/07/18 21:20:59, tapted wrote:
> On 2016/07/15 15:51:27, msw wrote:
> > I wonder if we intend to support these in Mac Views, and if they could just
> move
> > to chrome_browser_ui_views_non_chromeos_sources; +tapted@ for thoughts.
> > (profile_chooser_view_browsertest.cc comment: # This should be brought up on
> OSX
> > Views but not CrOS.)
> 
> ooh - yep! chrome_browser_ui_views_non_chromeos_sources is the right place for
> these. As it is, I think this will break the `compile` step with
> mac_views_browser=1 in GYP_DEFINES
> 
> chrome_browser_ui_views_non_chromeos_sources is already guarded by a similar
> condition, but with the fix for mac_views_browser=1
> 
>             ['chromeos == 0 and (OS!="mac" or mac_views_browser==1)', {
>               'sources': [ '<@(chrome_browser_ui_views_non_chromeos_sources)'
],
>             }],
> 
Done. Moved these classes to chrome_browser_ui_views_non_chromeos_sources. As
for profile_chooser_view_browertest.cc, I added "mac_views_browser==1" as a
condition.

https://codereview.chromium.org/2148293003/diff/1/chrome/chrome_browser_ui.gy...
chrome/chrome_browser_ui.gypi:3072:
'browser/ui/views/profiles/profile_chooser_view.h',
On 2016/07/15 15:51:27, msw wrote:
> You'll need to make includes of this header non-cros-specific.

Done. 
I had to do some refactoring in bookmark_bubble_sign_in_delegate_browsertest.cc
though for it to pass; if you think this is not worth it, I can revert back to
including profile_chooser_view.h at all times.

[tapted, 2016-07-19 05:42:30]

https://codereview.chromium.org/2148293003/diff/140001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/2148293003/diff/140001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:2651: ['toolkit_views==1 and (OS!="mac" or
mac_views_browser==1) and chromeos == 0', {
You'll need to modify chrome/test/BUILD.gn with something similar. E.g. to
something like


    if (toolkit_views) {
      sources += rebase_path(
              chrome_tests_gypi_values.chrome_browser_tests_views_sources,
              ".",
              "//chrome")
      deps += [ "//ui/views" ]
      if (!is_chromeos && (!is_mac || mac_views_browser)) {
        sources += rebase_path(
               
chrome_tests_gypi_values.chrome_browser_tests_views_non_cros_or_mac_sources,
                ".",
                "//chrome")
      }
      if (!is_mac) {
        sources += rebase_path(
               
chrome_tests_gypi_values.chrome_browser_tests_views_non_mac_sources,
                ".",
                "//chrome")
      }
    }



I tried this out and it compiles \o/. (and don't worry about
chrome_browser_tests_views_non_mac_sources - it's added elsewhere when
mac_views_browser=1)

[Jane, 2016-07-19 13:35:39]

https://codereview.chromium.org/2148293003/diff/140001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/2148293003/diff/140001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:2651: ['toolkit_views==1 and (OS!="mac" or
mac_views_browser==1) and chromeos == 0', {
On 2016/07/19 05:42:30, tapted wrote:
> You'll need to modify chrome/test/BUILD.gn with something similar. E.g. to
> something like
> 
> 
>     if (toolkit_views) {
>       sources += rebase_path(
>               chrome_tests_gypi_values.chrome_browser_tests_views_sources,
>               ".",
>               "//chrome")
>       deps += [ "//ui/views" ]
>       if (!is_chromeos && (!is_mac || mac_views_browser)) {
>         sources += rebase_path(
>                
> chrome_tests_gypi_values.chrome_browser_tests_views_non_cros_or_mac_sources,
>                 ".",
>                 "//chrome")
>       }
>       if (!is_mac) {
>         sources += rebase_path(
>                
> chrome_tests_gypi_values.chrome_browser_tests_views_non_mac_sources,
>                 ".",
>                 "//chrome")
>       }
>     }
> 
> 
> 
> I tried this out and it compiles \o/. (and don't worry about
> chrome_browser_tests_views_non_mac_sources - it's added elsewhere when
> mac_views_browser=1)

Done.

[Jane, 2016-07-19 13:58:23]

Patchset #6 (id:180001) has been deleted

[msw, 2016-07-19 18:51:05]

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:43:
void CreateRemoveSigninBrowser(Browser* last_active);
This name is a bit confusing, and it seems like you could easily leave the
implementation inlined within the BrowserRemoved fixture.

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:87:
if (switches::UsePasswordSeparatedSigninFlow()) {
optional nit: drop all these unnecessary curly braces.

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:129:
int starting_tab_count_normal = browser()->tab_strip_model()->count();
Leave this above, with the call to SignInBrowser, and the |tab_count_normal|
declaration, then just test they're equal in the non-cros case:

  int starting_tab_count_normal = browser()->tab_strip_model()->count();
  int starting_tab_count_incognito =
      incognito_browser->tab_strip_model()->count();

  SignInBrowser(incognito_browser);

  int tab_count_normal = browser()->tab_strip_model()->count();

#if !defined(OS_CHROMEOS)
  // ProfileChooser doesn't show in an incognito window.
  EXPECT_FALSE(ProfileChooserView::IsShowing());
  // No new tab should have been opened in the normal browser.
  EXPECT_EQ(starting_tab_count_normal, tab_count_normal);
#else
  // A new tab should have been opened in the normal browser.
  EXPECT_EQ(starting_tab_count_normal + 1, tab_count_normal);
#endif

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:157:
int starting_tab_count = extra_browser->tab_strip_model()->count();
Try to avoid some of this refactoring. You should be able to just replace the
|if (kHasProfileChooser)| condition with the |#if !defined(OS_CHROMEOS)|
preprocessor block; and test that |starting_tab_count| hasn't changed in the
OS_CHROMEOS case (or only define it in a separate |#if defined(OS_CHROMEOS)|
preprocessor block above); consider:

  int starting_tab_count = extra_browser->tab_strip_model()->count();

  std::unique_ptr<BubbleSyncPromoDelegate> delegate;
  delegate.reset(new BookmarkBubbleSignInDelegate(browser()));

  BrowserList::SetLastActive(extra_browser);

  // Close all tabs in the original browser.  Run all pending messages	
  // to make sure the browser window closes before continuing.
  browser()->tab_strip_model()->CloseAllTabs();
  content::RunAllPendingInMessageLoop();

  delegate->OnSignInLinkClicked();

  int tab_count = extra_browser->tab_strip_model()->count();
#if !defined(OS_CHROMEOS)
  if (switches::UsePasswordSeparatedSigninFlow())
    EXPECT_TRUE(extra_browser->signin_view_controller()->delegate());
  else
    EXPECT_TRUE(ProfileChooserView::IsShowing());
  // No new tab should have been opened in the extra browser.
  EXPECT_EQ(starting_tab_count, tab_count);
#else
  // A new tab should have been opened in the extra browser.
  EXPECT_EQ(starting_tab_count + 1, tab_count);
#endif

https://codereview.chromium.org/2148293003/diff/160001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/2148293003/diff/160001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:886:
'chrome_browser_tests_views_non_cros_or_mac_sources': [
Shouldn't this just be called chrome_browser_tests_views_non_cros_sources, since
now it *is* supported on mac views?

[Jane, 2016-07-19 19:48:55]

Patchset #6 (id:200001) has been deleted

[Jane, 2016-07-19 21:29:57]

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:43:
void CreateRemoveSigninBrowser(Browser* last_active);
On 2016/07/19 18:51:04, msw wrote:
> This name is a bit confusing, and it seems like you could easily leave the
> implementation inlined within the BrowserRemoved fixture.

Done. Removed this and put the code back.

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:87:
if (switches::UsePasswordSeparatedSigninFlow()) {
On 2016/07/19 18:51:04, msw wrote:
> optional nit: drop all these unnecessary curly braces.

Done.

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:129:
int starting_tab_count_normal = browser()->tab_strip_model()->count();
On 2016/07/19 18:51:04, msw wrote:
> Leave this above, with the call to SignInBrowser, and the |tab_count_normal|
> declaration, then just test they're equal in the non-cros case:
> 
>   int starting_tab_count_normal = browser()->tab_strip_model()->count();
>   int starting_tab_count_incognito =
>       incognito_browser->tab_strip_model()->count();
> 
>   SignInBrowser(incognito_browser);
> 
>   int tab_count_normal = browser()->tab_strip_model()->count();
> 
> #if !defined(OS_CHROMEOS)
>   // ProfileChooser doesn't show in an incognito window.
>   EXPECT_FALSE(ProfileChooserView::IsShowing());
>   // No new tab should have been opened in the normal browser.
>   EXPECT_EQ(starting_tab_count_normal, tab_count_normal);
> #else
>   // A new tab should have been opened in the normal browser.
>   EXPECT_EQ(starting_tab_count_normal + 1, tab_count_normal);
> #endif

Done.

https://codereview.chromium.org/2148293003/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:157:
int starting_tab_count = extra_browser->tab_strip_model()->count();
On 2016/07/19 18:51:04, msw wrote:
> Try to avoid some of this refactoring. You should be able to just replace the
> |if (kHasProfileChooser)| condition with the |#if !defined(OS_CHROMEOS)|
> preprocessor block; and test that |starting_tab_count| hasn't changed in the
> OS_CHROMEOS case (or only define it in a separate |#if defined(OS_CHROMEOS)|
> preprocessor block above); consider:
> 
>   int starting_tab_count = extra_browser->tab_strip_model()->count();
> 
>   std::unique_ptr<BubbleSyncPromoDelegate> delegate;
>   delegate.reset(new BookmarkBubbleSignInDelegate(browser()));
> 
>   BrowserList::SetLastActive(extra_browser);
> 
>   // Close all tabs in the original browser.  Run all pending messages	
>   // to make sure the browser window closes before continuing.
>   browser()->tab_strip_model()->CloseAllTabs();
>   content::RunAllPendingInMessageLoop();
> 
>   delegate->OnSignInLinkClicked();
> 
>   int tab_count = extra_browser->tab_strip_model()->count();
> #if !defined(OS_CHROMEOS)
>   if (switches::UsePasswordSeparatedSigninFlow())
>     EXPECT_TRUE(extra_browser->signin_view_controller()->delegate());
>   else
>     EXPECT_TRUE(ProfileChooserView::IsShowing());
>   // No new tab should have been opened in the extra browser.
>   EXPECT_EQ(starting_tab_count, tab_count);
> #else
>   // A new tab should have been opened in the extra browser.
>   EXPECT_EQ(starting_tab_count + 1, tab_count);
> #endif

Done. I did that much refactoring in an attempt to make all the ifdefs look
cleaner, but your way definitely looks better. Thanks for the suggestion!

https://codereview.chromium.org/2148293003/diff/160001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/2148293003/diff/160001/chrome/chrome_tests.gy...
chrome/chrome_tests.gypi:886:
'chrome_browser_tests_views_non_cros_or_mac_sources': [
On 2016/07/19 18:51:04, msw wrote:
> Shouldn't this just be called chrome_browser_tests_views_non_cros_sources,
since
> now it *is* supported on mac views?

Done.

[msw, 2016-07-19 21:42:05]

lgtm with a nit

https://codereview.chromium.org/2148293003/diff/220001/chrome/browser/ui/view...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/2148293003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:93:
EXPECT_EQ(starting_tab_count, browser()->tab_strip_model()->count());
optional nit: move this and line 95 outside the preprocessor block, since the
behavior is equivalent.

[Jane, 2016-07-19 22:09:14]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 22:10:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 23:08:05]

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

[commit-bot: I haz the power, 2016-07-19 23:08:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jane, 2016-07-19 23:13:31]

The CQ bit was checked by janeliulwq@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 23:14:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jane, 2016-07-19 23:15:11]

Heads up that the incognito browser test failed; I think tab count should be
(starting_tab_count_normal + 1) for both cases. Uploaded a newer patch for that.

https://codereview.chromium.org/2148293003/diff/220001/chrome/browser/ui/view...
File
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc
(right):

https://codereview.chromium.org/2148293003/diff/220001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_sign_in_delegate_browsertest.cc:93:
EXPECT_EQ(starting_tab_count, browser()->tab_strip_model()->count());
On 2016/07/19 21:42:04, msw wrote:
> optional nit: move this and line 95 outside the preprocessor block, since the
> behavior is equivalent.

Done.

[commit-bot: I haz the power, 2016-07-20 00:40:42]

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

[commit-bot: I haz the power, 2016-07-20 00:40:43]

Dry run: This issue passed the CQ dry run.

[Jane, 2016-07-20 01:15:12]

The CQ bit was checked by janeliulwq@google.com

[Jane, 2016-07-20 01:15:13]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2148293003/#ps260001
(title: "Fixed failed test")

[commit-bot: I haz the power, 2016-07-20 01:15:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 01:15:58]

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

[commit-bot: I haz the power, 2016-07-20 01:15:59]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Jane, 2016-07-20 01:21:36]

janeliulwq@google.com changed required reviewers:
- rogerta@chromium.org

[Jane, 2016-07-20 01:21:58]

The CQ bit was checked by janeliulwq@google.com

[commit-bot: I haz the power, 2016-07-20 01:22:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 01:38:09]

Description was changed from

==========
Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in
chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For
an example, see the build failures in
https://codereview.chromium.org/2151513002/#ps100001.
Tested by applying the above patch and verifying that the build passes.

BUG=615893
==========

to

==========
Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in
chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For
an example, see the build failures in
https://codereview.chromium.org/2151513002/#ps100001.
Tested by applying the above patch and verifying that the build passes.

BUG=615893
==========

[commit-bot: I haz the power, 2016-07-20 01:38:11]

Committed patchset #8 (id:260001)

[commit-bot: I haz the power, 2016-07-20 01:40:07]

Description was changed from

==========
Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in
chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For
an example, see the build failures in
https://codereview.chromium.org/2151513002/#ps100001.
Tested by applying the above patch and verifying that the build passes.

BUG=615893
==========

to

==========
Excluded non-chromeos source files in chromeos build
Excluded profile_chooser_view and profile_signin_confirmation_dialog_views in
chromeos builds because they are desktop-only views.
Building them in chromeos prevents them from using desktop-only functions. For
an example, see the build failures in
https://codereview.chromium.org/2151513002/#ps100001.
Tested by applying the above patch and verifying that the build passes.

BUG=615893

Committed: https://crrev.com/1831391aeba23ff66f87678b619ddee6113066d2
Cr-Commit-Position: refs/heads/master@{#406443}
==========

[commit-bot: I haz the power, 2016-07-20 01:40:09]

Patchset 8 (id:??) landed as
https://crrev.com/1831391aeba23ff66f87678b619ddee6113066d2
Cr-Commit-Position: refs/heads/master@{#406443}