Added notion of currently active app / browser

Added notion of currently active app / browser


BUG=115650
TEST=LauncherUpdaterTest.*, including new tests

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

[Avi (use Gerrit), 2012-03-13 15:07:37]

I get weird feelings about putting the idea of a root window on
RenderViewHostTester for Aura. I know this is just an accessor for it and that
the member variable is already there, but this makes no sense. Why would the
RenderViewHost need to be concerned about a root window? Why is this not needed
for any other platform?

https://chromiumcodereview.appspot.com/9689047/diff/7001/content/test/test_re...
File content/test/test_renderer_host.h (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/content/test/test_re...
content/test/test_renderer_host.h:152: aura::RootWindow* root_window() const {
return root_window_.get(); }
#if defined(USE_AURA)

[sky, 2012-03-13 17:14:42]

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/launcher/launche...
File ash/launcher/launcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/launcher/launche...
ash/launcher/launcher_unittest.cc:101: TEST_F(LauncherTest, OpenTwoBrowsers) {
Add a description of what this is intended to test.

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/launcher/launche...
ash/launcher/launcher_unittest.cc:107:
static_cast<LauncherView*>(contents_view->child_at(0));
This is fragile, I would rather see Launcher get a launcher_view_for_test()
member.

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/wm/window_util.c...
ash/wm/window_util.cc:37: DCHECK(window->GetRootWindow());
Maybe this shouldn't be a DCHECK, but instead return false. After all, if the
window doesn't have a rootwindow we know it's not active.

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:205: if (index
>= 0) {
Why is this conditional needed?

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:112:
launcher_delegate_->SetItemStatus(
Might tab be NULL here?

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:206: 
nit: remove empty line.

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:208: if (id >= 0) {
When would this return <= 0?

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:341:
ash::LauncherItemStatus status =
You have this code in three places. How about a GetStatusForTab function?

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/launcher_updater.h (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.h:36: class TestApi {
I hate seeing the first public thing in a class a set of test only methods, and
this code gets compiled into the release build. Can you instead add _test
methods instead, or if you really like the class approach put it in a separate
file that is only build for tests?

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.h:84: void
ActivationChanged(TabContentsWrapper* tab, bool active);
Don't group this with TabStripModel overrides.

Also, since this is similarly named to ActivateTabChanged and it takes
TabContentsWrapper I thought this meant the activation state of the tab changed.
Since LauncherUpdater knows about the model, can you remove the
TabContentsWrapper argument and instead name this something like
BrowserActivationStateChanged.

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/launcher_updater_unittest.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater_unittest.cc:622:
TEST_F(LauncherUpdaterTest, ActivateAppsInBrowser) {
Add descriptions for your tests.

[DaveMoore, 2012-03-13 23:33:56]

I think access to the root_window() in that class is appropriate for aura. On
other platforms there is no window that is the root of all others and they are
discovered from the platform if necessary. But aura is the platform and to mock
correct activation sequences we need a way to have the windows be parented.

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/launcher/launche...
File ash/launcher/launcher_unittest.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/launcher/launche...
ash/launcher/launcher_unittest.cc:101: TEST_F(LauncherTest, OpenTwoBrowsers) {
On 2012/03/13 17:14:42, sky wrote:
> Add a description of what this is intended to test.

Done.

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/launcher/launche...
ash/launcher/launcher_unittest.cc:107:
static_cast<LauncherView*>(contents_view->child_at(0));
On 2012/03/13 17:14:42, sky wrote:
> This is fragile, I would rather see Launcher get a launcher_view_for_test()
> member.

Done.

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/ash/wm/window_util.c...
ash/wm/window_util.cc:37: DCHECK(window->GetRootWindow());
On 2012/03/13 17:14:42, sky wrote:
> Maybe this shouldn't be a DCHECK, but instead return false. After all, if the
> window doesn't have a rootwindow we know it's not active.

Done.

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:112:
launcher_delegate_->SetItemStatus(
N/A after changing to BrowserActivationStateChanged()
On 2012/03/13 17:14:42, sky wrote:
> Might tab be NULL here?

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:206: 
On 2012/03/13 17:14:42, sky wrote:
> nit: remove empty line.

Done.

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:341:
ash::LauncherItemStatus status =
On 2012/03/13 17:14:42, sky wrote:
> You have this code in three places. How about a GetStatusForTab function?

Done.

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/launcher_updater.h (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.h:36: class TestApi {
I understand the issue about it being the first thing, I could put it lower but
the style guide says that nested classes come first. Wouldn't _test methods get
compiled for release as well, or is there some magic that keeps them from being
compiled too? An extra class w/ no virtual methods isn't any worse than some
methods is it?

I could put it in a separate file and make it a friend class. I like that it
lives in the same place as the code it deeply interacts with so it's more likely
to be kept in sync.

This is not the first instance of this pattern in our code (and they haven't all
come from me). Would you expect all of them to be changed as well?

On 2012/03/13 17:14:42, sky wrote:
> I hate seeing the first public thing in a class a set of test only methods,
and
> this code gets compiled into the release build. Can you instead add _test
> methods instead, or if you really like the class approach put it in a separate
> file that is only build for tests?

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.h:84: void
ActivationChanged(TabContentsWrapper* tab, bool active);
On 2012/03/13 17:14:42, sky wrote:
> Don't group this with TabStripModel overrides.
> 
> Also, since this is similarly named to ActivateTabChanged and it takes
> TabContentsWrapper I thought this meant the activation state of the tab
changed.
> Since LauncherUpdater knows about the model, can you remove the
> TabContentsWrapper argument and instead name this something like
> BrowserActivationStateChanged.

Done.

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/launcher_updater_unittest.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater_unittest.cc:622:
TEST_F(LauncherUpdaterTest, ActivateAppsInBrowser) {
On 2012/03/13 17:14:42, sky wrote:
> Add descriptions for your tests.

Done.

https://chromiumcodereview.appspot.com/9689047/diff/7001/content/test/test_re...
File content/test/test_renderer_host.h (right):

https://chromiumcodereview.appspot.com/9689047/diff/7001/content/test/test_re...
content/test/test_renderer_host.h:152: aura::RootWindow* root_window() const {
return root_window_.get(); }
On 2012/03/13 15:07:37, Avi wrote:
> #if defined(USE_AURA)

Done.

[Avi (use Gerrit), 2012-03-14 02:19:24]

I don't get it, but I'll withdraw.

[sky, 2012-03-14 04:05:44]

https://chromiumcodereview.appspot.com/9689047/diff/8001/ash/launcher/launche...
File ash/launcher/launcher.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/8001/ash/launcher/launche...
ash/launcher/launcher.cc:143: return static_cast<internal::LauncherView*>(
This is brittle, cache LauncherView as a field (I have a cl in that works that
needs it too).

https://chromiumcodereview.appspot.com/9689047/diff/8001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:205:
DCHECK(index >= 0);
DCHECK_GE

https://chromiumcodereview.appspot.com/9689047/diff/8001/chrome/browser/ui/vi...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:130:
launcher_delegate_->SetItemStatus(old_id, ash::STATUS_RUNNING);
nit: spacing

https://chromiumcodereview.appspot.com/9689047/diff/8001/chrome/browser/ui/vi...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:214: if (!tab)
Should this be before 209 now?

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
File ui/aura/test/test_activation_client.cc (right):

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
ui/aura/test/test_activation_client.cc:21:
TestActivationClient::~TestActivationClient() {
The destructor should remove all the observers.

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
ui/aura/test/test_activation_client.cc:38: if (last_active) {
nit: combine into a single if.

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
ui/aura/test/test_activation_client.cc:47: RemoveActiveWindow(window);
How come deactivate removes from the vector?

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
ui/aura/test/test_activation_client.cc:53: if (active_windows_.size() == 0)
empty

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
ui/aura/test/test_activation_client.cc:67: active_windows_.pop_back();
This should remove the observer too.

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
ui/aura/test/test_activation_client.cc:68: Window *next_active =
GetActiveWindow();
'window *' -> 'window* '

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
ui/aura/test/test_activation_client.cc:69: if (next_active) {
nit: combine into a single if.

https://chromiumcodereview.appspot.com/9689047/diff/8001/ui/aura/test/test_ac...
ui/aura/test/test_activation_client.cc:80: for (unsigned int i = 0; i <
active_windows_.size(); ++i) {
unsigned int -> size_t

[DaveMoore, 2012-03-14 19:54:58]

http://codereview.chromium.org/9689047/diff/8001/ash/launcher/launcher.cc
File ash/launcher/launcher.cc (right):

http://codereview.chromium.org/9689047/diff/8001/ash/launcher/launcher.cc#new...
ash/launcher/launcher.cc:143: return static_cast<internal::LauncherView*>(
On 2012/03/14 04:05:44, sky wrote:
> This is brittle, cache LauncherView as a field (I have a cl in that works that
> needs it too).

Done.

http://codereview.chromium.org/9689047/diff/8001/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9689047/diff/8001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:205:
DCHECK(index >= 0);
On 2012/03/14 04:05:44, sky wrote:
> DCHECK_GE

Done.

http://codereview.chromium.org/9689047/diff/8001/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9689047/diff/8001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:130:
launcher_delegate_->SetItemStatus(old_id, ash::STATUS_RUNNING);
On 2012/03/14 04:05:44, sky wrote:
> nit: spacing

Done.

http://codereview.chromium.org/9689047/diff/8001/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:214: if (!tab)
On 2012/03/14 04:05:44, sky wrote:
> Should this be before 209 now?

Done.

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
File ui/aura/test/test_activation_client.cc (right):

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
ui/aura/test/test_activation_client.cc:21:
TestActivationClient::~TestActivationClient() {
On 2012/03/14 04:05:44, sky wrote:
> The destructor should remove all the observers.

Done.

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
ui/aura/test/test_activation_client.cc:38: if (last_active) {
On 2012/03/14 04:05:44, sky wrote:
> nit: combine into a single if.

Done.

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
ui/aura/test/test_activation_client.cc:47: RemoveActiveWindow(window);
You're right...it shouldn't.
On 2012/03/14 04:05:44, sky wrote:
> How come deactivate removes from the vector?

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
ui/aura/test/test_activation_client.cc:53: if (active_windows_.size() == 0)
On 2012/03/14 04:05:44, sky wrote:
> empty

Done.

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
ui/aura/test/test_activation_client.cc:67: active_windows_.pop_back();
On 2012/03/14 04:05:44, sky wrote:
> This should remove the observer too.

Done.

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
ui/aura/test/test_activation_client.cc:68: Window *next_active =
GetActiveWindow();
On 2012/03/14 04:05:44, sky wrote:
> 'window *' -> 'window* '

Done.

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
ui/aura/test/test_activation_client.cc:69: if (next_active) {
On 2012/03/14 04:05:44, sky wrote:
> nit: combine into a single if.

Done.

http://codereview.chromium.org/9689047/diff/8001/ui/aura/test/test_activation...
ui/aura/test/test_activation_client.cc:80: for (unsigned int i = 0; i <
active_windows_.size(); ++i) {
On 2012/03/14 04:05:44, sky wrote:
> unsigned int -> size_t

Done.

[sky, 2012-03-14 20:54:39]

LGTM

[DaveMoore, 2012-03-14 21:43:28]

ben: content OWNER review please?

[Ben Goodger (Google), 2012-03-14 21:53:05]

content LGTM