views: Make view::Views::GetPreferredSize() const.

views: Make view::Views::GetPreferredSize() const.

Due to button refactoring patches, we now need to ask views for their
preferred sizes in const methods. Across the entire codebase, the
overwhelming majority of GetPreferredSize() implementations could be
const. There are only a few places where we can't, and they are usually
combining the sizing and layout code.

BookmarksBarView was simple enough to perform this separation. Most of
the other places were solved by adding "mutable" to value caches.

BUG=155363
R=jam@chromium.org, pkasting@chromium.org, scheib@chromium.org, sky@chromium.org, pkasting, sky
TBR=jam, scheib

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

[Elliot Glaysher, 2014-05-13 01:11:11]

This is a deceptively simple review; most of it is mechanical and you really
only need to review 6 files and a small directory.

.../ui/views/autofill/autofill_dialog_views.h
.../ui/views/bookmarks/bookmark_bar_view.cc
chrome/browser/ui/views/toolbar/wrench_menu.cc
ui/message_center/views/message_center_view.cc
ui/views/controls/styled_label.cc
ui/views/layout/*
ui/views/view.h

The rest is just adding const in a lot of places.

[Elliot Glaysher, 2014-05-13 01:12:23]

+msw, since he mentioned that he wanted to see this patch.

[sky, 2014-05-13 16:06:06]

I think you missed a bunch of places here. For example, you have modified some
.h files but not the corresponding .cc. Similarly you haven't updated all the
overrides to match.

https://codereview.chromium.org/273223002/diff/40001/ash/frame/custom_frame_v...
File ash/frame/custom_frame_view_ash.h (right):

https://codereview.chromium.org/273223002/diff/40001/ash/frame/custom_frame_v...
ash/frame/custom_frame_view_ash.h:58: virtual gfx::Size GetMinimumSize()
OVERRIDE;
const

https://codereview.chromium.org/273223002/diff/40001/ash/ime/candidate_view.cc
File ash/ime/candidate_view.cc (right):

https://codereview.chromium.org/273223002/diff/40001/ash/ime/candidate_view.c...
ash/ime/candidate_view.cc:275: gfx::Size CandidateView::GetPreferredSize() {
const

https://codereview.chromium.org/273223002/diff/40001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/273223002/diff/40001/ui/views/view.h#newcode279
ui/views/view.h:279: virtual gfx::Size GetMaximumSize();
Are you going to make this const at a later date?

[sky, 2014-05-13 16:13:55]

Two more.

https://codereview.chromium.org/273223002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/273223002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:667: gfx::Size
BookmarkBarView::GetPreferredSize() const {
The width of the bookmark bar is forced to that of the browser view. So, I think
you can simplify this to returning a width of 0 and the correct height.

https://codereview.chromium.org/273223002/diff/40001/ui/views/controls/styled...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/273223002/diff/40001/ui/views/controls/styled...
ui/views/controls/styled_label.cc:167: // I only feel OK about this quick hack
since this code is going away
Why is this code going away?

[Elliot Glaysher, 2014-05-14 00:48:56]

Recompiled locally with both chromeos=1 and not, catching a whole bunch of
compile problems.

In addition to the previous paths, ash/shelf/shelf_view.cc should be added to
the list of paths to review.

(It looks like this patch is now so big that the trybots are coughing on it. I
don't know what to do about that.)

https://codereview.chromium.org/273223002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/273223002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:667: gfx::Size
BookmarkBarView::GetPreferredSize() const {
On 2014/05/13 16:13:57, sky wrote:
> The width of the bookmark bar is forced to that of the browser view. So, I
think
> you can simplify this to returning a width of 0 and the correct height.

Done.

https://codereview.chromium.org/273223002/diff/40001/ui/views/controls/styled...
File ui/views/controls/styled_label.cc (right):

https://codereview.chromium.org/273223002/diff/40001/ui/views/controls/styled...
ui/views/controls/styled_label.cc:167: // I only feel OK about this quick hack
since this code is going away
On 2014/05/13 16:13:57, sky wrote:
> Why is this code going away?

I actually got the wrong impression from msw@. This code has no plans to go
away.

I still don't like it, but I guess a const_cast<> here isn't the end of the
world.

https://codereview.chromium.org/273223002/diff/40001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/273223002/diff/40001/ui/views/view.h#newcode279
ui/views/view.h:279: virtual gfx::Size GetMaximumSize();
On 2014/05/13 16:06:08, sky wrote:
> Are you going to make this const at a later date?

I do intend to. However, this patch now touches 396 files after combing through
the ash/ directory for things that I previously missed.

[sky, 2014-05-14 16:53:33]

LGTM

[Elliot Glaysher, 2014-05-14 20:38:27]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-05-14 20:39:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/273223002/160001

[Elliot Glaysher, 2014-05-14 21:57:50]

The CQ bit was unchecked by erg@chromium.org

[Elliot Glaysher, 2014-05-14 22:00:11]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-05-14 22:01:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/273223002/180001

[commit-bot: I haz the power, 2014-05-15 02:11:42]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-15 02:19:02]

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

[commit-bot: I haz the power, 2014-05-15 02:19:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Elliot Glaysher, 2014-05-15 17:56:09]

pkasting: please review components/infobars/. This patch is going through,
trying to make methods on View const.

scheib,jam: this is TBRed to you; in files in the directories you own, there are
only mechanical changes.

[Peter Kasting, 2014-05-15 18:30:24]

https://codereview.chromium.org/273223002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/extension_infobar.cc (right):

https://codereview.chromium.org/273223002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/extension_infobar.cc:174:
GetDelegate()->extension_view_host()->view()->GetMinimumSize().width();
If you simply inline "delegate()->AsExtensionInfoBarDelegate()" in place of
"GetDelegate()" here, then you won't need to add a const form of GetDelegate(),
nor will you need to add the const version of
InfoBarDelegate::AsExtensionInfoBarDelegate().

https://codereview.chromium.org/273223002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/273223002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.h:236: const
OmniboxViewViews* omnibox_view() const { return omnibox_view_; }
What actually uses this?

[jam, 2014-05-15 19:49:46]

content lgtm

[Elliot Glaysher, 2014-05-15 19:56:34]

https://codereview.chromium.org/273223002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/infobars/extension_infobar.cc (right):

https://codereview.chromium.org/273223002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/infobars/extension_infobar.cc:174:
GetDelegate()->extension_view_host()->view()->GetMinimumSize().width();
On 2014/05/15 18:30:26, Peter Kasting wrote:
> If you simply inline "delegate()->AsExtensionInfoBarDelegate()" in place of
> "GetDelegate()" here, then you won't need to add a const form of
GetDelegate(),
> nor will you need to add the const version of
> InfoBarDelegate::AsExtensionInfoBarDelegate().

Done.

https://codereview.chromium.org/273223002/diff/180001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/273223002/diff/180001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.h:236: const
OmniboxViewViews* omnibox_view() const { return omnibox_view_; }
On 2014/05/15 18:30:26, Peter Kasting wrote:
> What actually uses this?

chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:594:22

return gfx::Rect(location_bar_view->GetOmniboxViewOrigin(),
                 gfx::Size(0, location_bar_view->omnibox_view()->height()));

[Peter Kasting, 2014-05-15 20:27:41]

LGTM

[Elliot Glaysher, 2014-05-15 22:17:46]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-05-15 22:19:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/273223002/220001

[commit-bot: I haz the power, 2014-05-16 01:34:31]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-16 01:47:26]

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

[commit-bot: I haz the power, 2014-05-16 01:47:26]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[scheib, 2014-05-16 16:32:29]

apps/* lgtm

[Elliot Glaysher, 2014-05-16 17:38:20]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-05-16 17:38:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/273223002/220001

[commit-bot: I haz the power, 2014-05-16 17:54:04]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-16 18:08:02]

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

[commit-bot: I haz the power, 2014-05-16 18:08:03]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Elliot Glaysher, 2014-05-19 20:47:11]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2014-05-19 20:48:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erg@chromium.org/273223002/260001

[commit-bot: I haz the power, 2014-05-20 00:47:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-20 01:15:05]

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

[commit-bot: I haz the power, 2014-05-20 01:15:06]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[Elliot Glaysher, 2014-05-20 17:24:59]

Committed patchset #14 manually as r271682 (presubmit successful).