[Toolbar Views] Move toolbar files to a new subdirectory.

[Toolbar Views] Move toolbar files to a new subdirectory.

This mimics the organization of chrome/browser/ui as well.

R=pkasting@chromium.org

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

[tfarina, 2013-11-13 17:24:25]

That is cool! Thanks for doing this. It makes easier to reason about the code
and to see which files compose the "toolbar", even if it isn't the traditional
toolbar we see in other softwares.

[tfarina, 2013-11-13 17:27:01]

https://codereview.chromium.org/71743002/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/DEPS (right):

https://codereview.chromium.org/71743002/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/DEPS:8:
"!chrome/browser/ui/views/location_bar/location_icon_view.h",
This makes me think, if now we have a toolbar directory, isn't location_bar (the
omnibox) inside the toolbar? of course, I'm not suggesting moving it in this CL
nor in a follow up. I'm just thinking aloud.

[tfarina, 2013-11-13 17:29:29]

https://codereview.chromium.org/71743002/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar/wrench_toolbar_button.h (right):

https://codereview.chromium.org/71743002/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar/wrench_toolbar_button.h:11: class
WrenchToolbarButton : public views::MenuButton,
can we name this file just wrench_button.h, now that it lives in toolbar
directory? Also I think Wrench is misleading since we replaced the wrench icon
by the hotdog one a long time ago.

May be we could name it just AppButton? I don't have a better naming suggestion
though.

[Greg Billock, 2013-11-13 18:23:26]

https://codereview.chromium.org/71743002/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/DEPS (right):

https://codereview.chromium.org/71743002/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/DEPS:8:
"!chrome/browser/ui/views/location_bar/location_icon_view.h",
On 2013/11/13 17:27:01, tfarina wrote:
> This makes me think, if now we have a toolbar directory, isn't location_bar
(the
> omnibox) inside the toolbar? of course, I'm not suggesting moving it in this
CL
> nor in a follow up. I'm just thinking aloud.

I pondered that, but the location bar is rather complex, and keeping the current
organization mirrors other spots in the code as well. The
chrome/browser/ui/views directory is rather full, though, so pulling these out
is nice.

https://codereview.chromium.org/71743002/diff/1/chrome/browser/ui/views/toolb...
File chrome/browser/ui/views/toolbar/wrench_toolbar_button.h (right):

https://codereview.chromium.org/71743002/diff/1/chrome/browser/ui/views/toolb...
chrome/browser/ui/views/toolbar/wrench_toolbar_button.h:11: class
WrenchToolbarButton : public views::MenuButton,
On 2013/11/13 17:29:30, tfarina wrote:
> can we name this file just wrench_button.h, now that it lives in toolbar
> directory? Also I think Wrench is misleading since we replaced the wrench icon
> by the hotdog one a long time ago.
> 
> May be we could name it just AppButton? I don't have a better naming
suggestion
> though.

There's a couple good wrench refactors. Let's do them in a separate cl. (Marked
one with a TODO.)

[Peter Kasting, 2013-11-13 19:35:27]

I agree this is nice.  Rubber-stamp LGTM assuming you didn't change anything
other than moving stuff around and fixing up the resulting references.

[Greg Billock, 2013-11-13 19:56:21]

On 2013/11/13 19:35:27, Peter Kasting wrote:
> I agree this is nice.  Rubber-stamp LGTM assuming you didn't change anything
> other than moving stuff around and fixing up the resulting references.

That's right. This is purely a move with no changes other than #include and
#define lines. (Well, one todo comment... :-))

[commit-bot: I haz the power, 2013-11-13 19:57:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/1

[commit-bot: I haz the power, 2013-11-13 20:24:32]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Greg Billock, 2013-11-13 20:28:27]

On 2013/11/13 20:24:32, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

+piman for chromeos owners

[DaveMoore, 2013-11-13 20:57:24]

lgtm

[commit-bot: I haz the power, 2013-11-13 21:05:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/1

[tfarina, 2013-11-13 21:06:31]

Greg, looks like you haven`t upload patch set 2 with the TODO.

[piman, 2013-11-13 22:14:20]

lgtm

[commit-bot: I haz the power, 2013-11-13 22:45:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-14 17:22:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/300001

[commit-bot: I haz the power, 2013-11-14 17:23:06]

Failed to apply patch for
chrome/browser/ui/views/location_bar/star_view_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/location_bar/star_view_browsertest.cc
  Hunk #1 FAILED at 6.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/views/location_bar/star_view_browsertest.cc.rej

Patch:       chrome/browser/ui/views/location_bar/star_view_browsertest.cc
Index: chrome/browser/ui/views/location_bar/star_view_browsertest.cc
diff --git a/chrome/browser/ui/views/location_bar/star_view_browsertest.cc
b/chrome/browser/ui/views/location_bar/star_view_browsertest.cc
index
4e25c14a20f3261e9d140c3dfa2652f8ee494b27..fe0ecd6d8586bed2d9144b20d545100630cd8f63
100644
--- a/chrome/browser/ui/views/location_bar/star_view_browsertest.cc
+++ b/chrome/browser/ui/views/location_bar/star_view_browsertest.cc
@@ -6,7 +6,7 @@
 
 #include "chrome/browser/ui/views/bookmarks/bookmark_bubble_view.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
-#include "chrome/browser/ui/views/toolbar_view.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/test/base/in_process_browser_test.h"
 
 namespace {

[commit-bot: I haz the power, 2013-11-14 18:44:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/330001

[commit-bot: I haz the power, 2013-11-14 21:20:13]

Retried try job too often on win_x64_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-11-15 02:28:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/610001

[commit-bot: I haz the power, 2013-11-15 04:23:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/610001

[commit-bot: I haz the power, 2013-11-15 07:34:14]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-15 07:39:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/610001

[commit-bot: I haz the power, 2013-11-15 10:26:22]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-15 15:33:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/610001

[commit-bot: I haz the power, 2013-11-15 22:25:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/610001

[commit-bot: I haz the power, 2013-11-16 01:06:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/610001

[commit-bot: I haz the power, 2013-11-16 01:06:36]

Failed to apply patch for
chrome/browser/ui/views/toolbar/browser_action_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         chrome/browser/ui/views/toolbar/browser_action_view.cc
  Copied chrome/browser/ui/views/browser_action_view.cc ->
chrome/browser/ui/views/toolbar/browser_action_view.cc
  patching file chrome/browser/ui/views/toolbar/browser_action_view.cc
  Hunk #2 FAILED at 14.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/toolbar/browser_action_view.cc.rej

Patch:   NR 
chrome/browser/ui/views/browser_action_view.cc->chrome/browser/ui/views/toolbar/browser_action_view.cc
Index: chrome/browser/ui/views/toolbar/browser_action_view.cc
diff --git a/chrome/browser/ui/views/browser_action_view.cc
b/chrome/browser/ui/views/toolbar/browser_action_view.cc
similarity index 98%
rename from chrome/browser/ui/views/browser_action_view.cc
rename to chrome/browser/ui/views/toolbar/browser_action_view.cc
index
75705c7a96ef45aa3c91e61129f22361775dac8e..54433173080f4d42bbe1a57e7eb85c48971b121a
100644
--- a/chrome/browser/ui/views/browser_action_view.cc
+++ b/chrome/browser/ui/views/toolbar/browser_action_view.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/views/browser_action_view.h"
+#include "chrome/browser/ui/views/toolbar/browser_action_view.h"
 
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -14,8 +14,8 @@
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/views/browser_actions_container.h"
-#include "chrome/browser/ui/views/toolbar_view.h"
+#include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
+#include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/common/extensions/extension.h"
 #include "extensions/common/manifest_constants.h"
 #include "grit/generated_resources.h"

[commit-bot: I haz the power, 2013-11-16 06:41:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/71743002/1080001

[commit-bot: I haz the power, 2013-11-18 08:55:15]

Change committed as 235677

[tfarina, 2013-11-22 22:21:23]

https://codereview.chromium.org/71743002/diff/1080001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu_observer.h (right):

https://codereview.chromium.org/71743002/diff/1080001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu_observer.h:8: // TODO(gbillock):
Make this an inner class of WrenchMenu. (even needed?)
why did you add this TODO?

Quoting from chromium coding style:

"Prefer putting delegate classes in their own header files. Implementors of the
delegate interface will often be included elsewhere, which will often cause more
coupling with the header of the main class."

http://www.chromium.org/developers/coding-style - Code formatting section

We also discussed about this in https://codereview.chromium.org/79773005/