Removing pre-material design menu setting.

Removing pre-material design menu setting.

BUG=694420
Review-Url: https://codereview.chromium.org/2708073002
Cr-Commit-Position: refs/heads/master@{#455113}
Committed: https://chromium.googlesource.com/chromium/src/+/90c1986a3559d1f57ae92ff654bcd5a64a41e96f

[jlebel, 2017-02-22 10:07:55]

Description was changed from

==========
Removing material design menu setting.

BUG=
==========

to

==========
Removing pre-material design menu setting.

BUG=
==========

[jlebel, 2017-02-22 12:21:15]

Patchset #1 (id:1) has been deleted

[jlebel, 2017-02-22 12:21:24]

Patchset #1 (id:20001) has been deleted

[jlebel, 2017-02-22 12:24:05]

Patchset #1 (id:40001) has been deleted

[jlebel, 2017-02-22 12:24:34]

Description was changed from

==========
Removing pre-material design menu setting.

BUG=
==========

to

==========
Removing pre-material design menu setting.

BUG=694420
==========

[jlebel, 2017-02-22 12:30:06]

Hello Mihai,

Can you review this patch?

Thanks,

[jlebel, 2017-02-22 17:49:36]

jlebel@chromium.org changed reviewers:
+ msarda@chromium.org

[jlebel, 2017-02-22 17:49:37]

Hello Mihai,

Can you review this patch?

Thanks,

[msarda, 2017-02-23 11:51:03]

LGTM with nits.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:122: CGFloat
GetFixedMenuWidth() {
Nit: This should not be a constant named kFixedMenuWidth

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2181: int
iconImageSide = 18;
Optional nit: Let's make this a constant:
const int kIcontImageSide = 18;

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:114: int
GetFixedMenuWidth() {
Same here: Consider making these constants

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:370:
set_can_process_events_within_subtree(false);
Why this change?

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:422: icon_size = 22;
consider making this a constant.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:423: icon_color =
gfx::kChromeIconGrey;
I think this variable can be removed now.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:432: static int
icon_image_side() { return kMdImageSide; }
Can this this function be replaced by a const?

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:434: static int
badge_spacing() {
Can this this function be replaced by a const?

[jlebel, 2017-02-23 18:18:50]

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:122: CGFloat
GetFixedMenuWidth() {
On 2017/02/23 11:51:02, msarda wrote:
> Nit: This should not be a constant named kFixedMenuWidth

Done.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:2181: int
iconImageSide = 18;
On 2017/02/23 11:51:02, msarda wrote:
> Optional nit: Let's make this a constant:
> const int kIcontImageSide = 18;

Done.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:114: int
GetFixedMenuWidth() {
On 2017/02/23 11:51:02, msarda wrote:
> Same here: Consider making these constants

Done.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:370:
set_can_process_events_within_subtree(false);
On 2017/02/23 11:51:02, msarda wrote:
> Why this change?

To replace:
bool CanProcessEventsWithinSubtree() const override { return interactive_; }

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:422: icon_size = 22;
On 2017/02/23 11:51:03, msarda wrote:
> consider making this a constant.

Done.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:423: icon_color =
gfx::kChromeIconGrey;
On 2017/02/23 11:51:02, msarda wrote:
> I think this variable can be removed now.

Done.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:432: static int
icon_image_side() { return kMdImageSide; }
On 2017/02/23 11:51:02, msarda wrote:
> Can this this function be replaced by a const?

Done.

https://codereview.chromium.org/2708073002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:434: static int
badge_spacing() {
On 2017/02/23 11:51:03, msarda wrote:
> Can this this function be replaced by a const?

Done.

[jlebel, 2017-02-23 18:22:05]

jlebel@chromium.org changed reviewers:
+ jochen@chromium.org, mark@chromium.org, sky@chromium.org

[jlebel, 2017-02-23 18:22:06]

Hello Mark, Jochen and Sky,


I have this big patch to remove pre-material design menu
setting. Can you review it?

Thanks,

mark:
  chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
  chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
  chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
jochen:
  chrome/browser/sync/sync_global_error.cc
  chrome/browser/sync/sync_global_error.h
  chrome/browser/sync/sync_global_error_factory.cc
  chrome/browser/sync/sync_global_error_unittest.cc
  chrome/browser/ui/avatar_button_error_controller.cc
  chrome/browser/ui/avatar_button_error_controller.h
  chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
  chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
  chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
  chrome/browser/ui/views/profiles/new_avatar_button.cc
  chrome/browser/ui/views/profiles/profile_chooser_view.cc
  chrome/browser/ui/views/profiles/profile_chooser_view.h
  chrome/browser/ui/webui/signin/login_ui_service.cc
  components/signin/core/common/profile_management_switches.cc
  components/signin/core/common/profile_management_switches.h
  components/signin/core/common/signin_switches.cc
  components/signin/core/common/signin_switches.h
sky:
  chrome/browser/sync/sync_global_error.cc
  chrome/browser/sync/sync_global_error.h
  chrome/browser/sync/sync_global_error_factory.cc
  chrome/browser/sync/sync_global_error_unittest.cc
  chrome/browser/ui/avatar_button_error_controller.cc
  chrome/browser/ui/avatar_button_error_controller.h
  chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm
  chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm
  chrome/browser/ui/cocoa/profiles/profile_chooser_controller_unittest.mm
  chrome/browser/ui/views/profiles/new_avatar_button.cc
  chrome/browser/ui/views/profiles/profile_chooser_view.cc
  chrome/browser/ui/views/profiles/profile_chooser_view.h
  chrome/browser/ui/webui/signin/login_ui_service.cc

[sky, 2017-02-23 20:44:52]

Make sure you prune stale includes too. You've listed a bunch of us for the same
files. I'm assuming Mark will look at the cocoa files.

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error.cc (left):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error.cc:33:
global_error_service_->AddUnownedGlobalError(this);
Who owns this class now?

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error.cc (right):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error.cc:12: #include
"chrome/browser/ui/global_error/global_error_service.h"
Remove include?

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error_factory.cc (right):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error_factory.cc:11: #include
"chrome/browser/ui/global_error/global_error_service_factory.h"
Is this include needed anymore?

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/avata...
File chrome/browser/ui/avatar_button_error_controller.cc (right):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/avata...
chrome/browser/ui/avatar_button_error_controller.cc:77: void
AvatarButtonErrorController::SyncErrorObserver::OnErrorChanged() {
Is this function ever called now? Who adds this class as an observer now? If no
one does, does it need to implement SigninErrorController::Observer?

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:373:
set_can_process_events_within_subtree(false);
Why are you adding this?

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:554: if (view_mode ==
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER) {
no {}

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1037:
CreateMaterialDesignCurrentProfileView(item, false);
Now that MaterialDesign is the default, can functions with material in the name
be renamed? e.g. CreateCurrentProfileView.

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1038: if
(!IsProfileChooser(view_mode_)) {
no {}

[jochen (gone - plz use gerrit), 2017-02-24 16:00:29]

deferring to sky@

[sky, 2017-02-24 18:32:40]

Jochen, I'm not an owner of components/signin, so you'll need to
review that one.

On Fri, Feb 24, 2017 at 8:00 AM,  <jochen@chromium.org> wrote:
> deferring to sky@
>
> https://codereview.chromium.org/2708073002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msarda, 2017-02-27 15:38:17]

On 2017/02/24 18:32:40, sky wrote:
> Jochen, I'm not an owner of components/signin, so you'll need to
> review that one.
> 
> On Fri, Feb 24, 2017 at 8:00 AM,  <mailto:jochen@chromium.org> wrote:
> > deferring to sky@
> >
> > https://codereview.chromium.org/2708073002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

I am an owner of components/signin and I have reviewed that part already.

[jlebel, 2017-03-03 15:21:20]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 15:21:36]

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, 2017-03-03 15:23:47]

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

[commit-bot: I haz the power, 2017-03-03 15:23:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jlebel, 2017-03-06 12:46:06]

Patchset #4 (id:120001) has been deleted

[jlebel, 2017-03-06 13:09:30]

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error.cc (left):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error.cc:33:
global_error_service_->AddUnownedGlobalError(this);
On 2017/02/23 20:44:52, sky wrote:
> Who owns this class now?

Are you talking about SyncGlobalError?

SyncGlobalError is a PKS, so it basically destroyed when the profile is
destroyed.

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error.cc (right):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error.cc:12: #include
"chrome/browser/ui/global_error/global_error_service.h"
On 2017/02/23 20:44:52, sky wrote:
> Remove include?

Done.

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error_factory.cc (right):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error_factory.cc:11: #include
"chrome/browser/ui/global_error/global_error_service_factory.h"
On 2017/02/23 20:44:52, sky wrote:
> Is this include needed anymore?

Done.

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/avata...
File chrome/browser/ui/avatar_button_error_controller.cc (right):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/avata...
chrome/browser/ui/avatar_button_error_controller.cc:77: void
AvatarButtonErrorController::SyncErrorObserver::OnErrorChanged() {
On 2017/02/23 20:44:52, sky wrote:
> Is this function ever called now? Who adds this class as an observer now? If
no
> one does, does it need to implement SigninErrorController::Observer?

Sorry I definitely made a mistake here. GetSyncErrorControllerIfNeeded() should
not have been removed.

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:373:
set_can_process_events_within_subtree(false);
On 2017/02/23 20:44:52, sky wrote:
> Why are you adding this?

This is to replace:
bool CanProcessEventsWithinSubtree() const override { return interactive_; }

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:554: if (view_mode ==
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER) {
On 2017/02/23 20:44:52, sky wrote:
> no {}

Done.

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1037:
CreateMaterialDesignCurrentProfileView(item, false);
On 2017/02/23 20:44:52, sky wrote:
> Now that MaterialDesign is the default, can functions with material in the
name
> be renamed? e.g. CreateCurrentProfileView.

Done.

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1038: if
(!IsProfileChooser(view_mode_)) {
On 2017/02/23 20:44:52, sky wrote:
> no {}

Done.

[jlebel, 2017-03-06 13:11:39]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 13:11:55]

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, 2017-03-06 13:38:47]

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

[commit-bot: I haz the power, 2017-03-06 13:38:47]

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

[Mark Mentovai, 2017-03-06 16:31:39]

LGTM. I only reviewed the .mm files.

[jlebel, 2017-03-06 16:43:59]

jlebel@chromium.org changed reviewers:
- jochen@chromium.org

[jlebel, 2017-03-06 17:53:08]

The CQ bit was checked by jlebel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 17:53:41]

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

[sky, 2017-03-06 18:50:41]

LGTM

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error.cc (left):

https://codereview.chromium.org/2708073002/diff/80001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error.cc:33:
global_error_service_->AddUnownedGlobalError(this);
On 2017/03/06 13:09:29, jlebel wrote:
> On 2017/02/23 20:44:52, sky wrote:
> > Who owns this class now?
> 
> Are you talking about SyncGlobalError?
> 
> SyncGlobalError is a PKS, so it basically destroyed when the profile is
> destroyed.

Acknowledged.

https://codereview.chromium.org/2708073002/diff/160001/chrome/browser/ui/avat...
File chrome/browser/ui/avatar_button_error_controller.cc (right):

https://codereview.chromium.org/2708073002/diff/160001/chrome/browser/ui/avat...
chrome/browser/ui/avatar_button_error_controller.cc:11: #include
"components/signin/core/common/profile_management_switches.h"
Is this still needed?

[commit-bot: I haz the power, 2017-03-06 19:43:27]

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

[commit-bot: I haz the power, 2017-03-06 19:43:28]

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_...)

[jlebel, 2017-03-06 22:36:24]

Thanks sky.

Can you give me the list of files you reviewed so I can find files I still need
to get a review (since Mark accepted to review only .mm files)?

Thanks,

https://codereview.chromium.org/2708073002/diff/160001/chrome/browser/ui/avat...
File chrome/browser/ui/avatar_button_error_controller.cc (right):

https://codereview.chromium.org/2708073002/diff/160001/chrome/browser/ui/avat...
chrome/browser/ui/avatar_button_error_controller.cc:11: #include
"components/signin/core/common/profile_management_switches.h"
On 2017/03/06 18:50:40, sky wrote:
> Is this still needed?

Done.

[msarda, 2017-03-07 08:54:09]

Jerome,

I think it is fine to land this CL: both me and sky@ have reviewed it entirely
and L-G-T-M-ed it and mark@ has reviewed and L-G-T-M-ed the objective-c files. I
do not think you need any more reviews on this CL.

[jlebel, 2017-03-07 09:37:11]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-03-07 09:37:12]

The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org,
mark@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2708073002/#ps180001
(title: "Removing header")

[commit-bot: I haz the power, 2017-03-07 09:37:27]

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, 2017-03-07 10:39:54]

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

[commit-bot: I haz the power, 2017-03-07 10:39:56]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jlebel, 2017-03-07 15:52:38]

The CQ bit was checked by jlebel@chromium.org

[commit-bot: I haz the power, 2017-03-07 15:53:43]

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, 2017-03-07 17:24:38]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1488901958563800,
"parent_rev": "94aeb8cb883faee716cca33b7601df68a2d8c09c", "commit_rev":
"90c1986a3559d1f57ae92ff654bcd5a64a41e96f"}

[commit-bot: I haz the power, 2017-03-07 17:25:29]

Description was changed from

==========
Removing pre-material design menu setting.

BUG=694420
==========

to

==========
Removing pre-material design menu setting.

BUG=694420

Review-Url: https://codereview.chromium.org/2708073002
Cr-Commit-Position: refs/heads/master@{#455113}
Committed:
https://chromium.googlesource.com/chromium/src/+/90c1986a3559d1f57ae92ff654bc...
==========

[commit-bot: I haz the power, 2017-03-07 17:25:30]

Committed patchset #6 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/90c1986a3559d1f57ae92ff654bc...