Handling browser item status in BrowserShortcutLauncherItemController

Handling browser item status in BrowserShortcutLauncherItemController

Move the role of handling browser item status from ChromeLauncherController
to BrowserShortcutLauncherItemController.

R=skuhne@chromium.org
BUG=NONE
TEST=browser_tests, unit_tests

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

[simonhong_, 2013-08-29 20:36:39]

Dear stefan,
Please take a look!
Thank you.
Simon.

[simonhong_, 2013-09-03 23:50:49]

kindly ping..

[Mr4D (OOO till 08-26), 2013-09-04 15:12:33]

Thanks for doing this!

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:55:
// This check needs for win7_aura. UpdateBrowserItemStatus() access Shell.
Maybe something like:

// The shell will not be available for win7_aura unittests like 
// ChromeLauncherControllerTest.BrowserMenuGeneration.

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:75:
GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
Coincidentally the "UpdateAppState" goes hand in hand with
"GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus()" - so why
not calling UpdateBrowserItemStatus from "UpdateAppState"? and remove the many
calls you have here?

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:208:
BrowserStatusMonitor::GetBrowserShortcutLauncherItemController() {
You need this function only to get access to the "UpdateBrowserItemStatus"
function - so why not getting rid of this function and add a
"UpdateBrowserItemStatus()" instead which gets the CLC and calls the function?

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1508: // exist,
create it.
What about:

// Create a LauncherItemController for the Browser shortcut if it does not exist
yet.

[simonhong_, 2013-09-04 17:08:30]

Dear stefan,
Please check my comments.
Thank you for review!

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
(right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:55:
// This check needs for win7_aura. UpdateBrowserItemStatus() access Shell.
On 2013/09/04 15:12:33, Mr4D wrote:
> Maybe something like:
> 
> // The shell will not be available for win7_aura unittests like 
> // ChromeLauncherControllerTest.BrowserMenuGeneration.

Done.

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:75:
GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
On 2013/09/04 15:12:33, Mr4D wrote:
> Coincidentally the "UpdateAppState" goes hand in hand with
> "GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus()" - so
why
> not calling UpdateBrowserItemStatus from "UpdateAppState"? and remove the many
> calls you have here?

Yes, it can call by UpdateAppState() and it has same effect.
But, I thought the code that update browser item status should exist in BSLIC
because BSLIC handles launcher browser item status.

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:208:
BrowserStatusMonitor::GetBrowserShortcutLauncherItemController() {
On 2013/09/04 15:12:33, Mr4D wrote:
> You need this function only to get access to the "UpdateBrowserItemStatus"
> function - so why not getting rid of this function and add a
> "UpdateBrowserItemStatus()" instead which gets the CLC and calls the function?

Same reason as above comments.
This class is just monitoring class of browser status.
So I thought locating UpdateBrowserItemStatus() in BSLIC looks better.
If you want to call UpdateBrowserItemStatus() in UpdateAppState(), I'll follow
you.

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1508: // exist,
create it.
On 2013/09/04 15:12:33, Mr4D wrote:
> What about:
> 
> // Create a LauncherItemController for the Browser shortcut if it does not
exist
> yet.

Done.

[simonhong_, 2013-09-04 23:46:35]

On 2013/09/04 17:08:30, Simon YoungKi Hong wrote:
> Dear stefan,
> Please check my comments.
> Thank you for review!
> 
>
https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
> File
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc
> (right):
> 
>
https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
>
chrome/browser/ui/ash/launcher/browser_shortcut_launcher_item_controller.cc:55:
> // This check needs for win7_aura. UpdateBrowserItemStatus() access Shell.
> On 2013/09/04 15:12:33, Mr4D wrote:
> > Maybe something like:
> > 
> > // The shell will not be available for win7_aura unittests like 
> > // ChromeLauncherControllerTest.BrowserMenuGeneration.
> 
> Done.
> 
>
https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
> File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):
> 
>
https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
> chrome/browser/ui/ash/launcher/browser_status_monitor.cc:75:
> GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
> On 2013/09/04 15:12:33, Mr4D wrote:
> > Coincidentally the "UpdateAppState" goes hand in hand with
> > "GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus()" - so
> why
> > not calling UpdateBrowserItemStatus from "UpdateAppState"? and remove the
many
> > calls you have here?
> 
> Yes, it can call by UpdateAppState() and it has same effect.
> But, I thought the code that update browser item status should exist in BSLIC
> because BSLIC handles launcher browser item status.
> 
>
https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
> chrome/browser/ui/ash/launcher/browser_status_monitor.cc:208:
> BrowserStatusMonitor::GetBrowserShortcutLauncherItemController() {
> On 2013/09/04 15:12:33, Mr4D wrote:
> > You need this function only to get access to the "UpdateBrowserItemStatus"
> > function - so why not getting rid of this function and add a
> > "UpdateBrowserItemStatus()" instead which gets the CLC and calls the
function?
> 
> Same reason as above comments.
> This class is just monitoring class of browser status.
> So I thought locating UpdateBrowserItemStatus() in BSLIC looks better.
> If you want to call UpdateBrowserItemStatus() in UpdateAppState(), I'll follow
> you.
> 
>
https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
> File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):
> 
>
https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
> chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:1508: // exist,
> create it.
> On 2013/09/04 15:12:33, Mr4D wrote:
> > What about:
> > 
> > // Create a LauncherItemController for the Browser shortcut if it does not
> exist
> > yet.
> 
> Done.

My initial purpose of this cl was updating browser item status independently
regardless of other apps item status.
To do that, I was trying to monitor browser list same as
BrowserStatusMonitor(BSM).
But most of codes are duplicated with BSM.
So I just call update from BSM.
This is background of this CL. :)

[Mr4D (OOO till 08-26), 2013-09-05 23:30:12]

Please see my comments!

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:75:
GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
You misunderstood - I was wondering why UpdateAppState(..) isn't at the same
time calling the UpdateBrowserItemStatus. This was maybe a bad example, but when
you look into line 153, 160, 167, 175, 186 you will notice that the two calls
always come in pairs...

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:208:
BrowserStatusMonitor::GetBrowserShortcutLauncherItemController() {
You have misunderstood:

void BrowserStatusMonitor::UpdateBrowserItemStatus() {
 
launcher_controller_->GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
}

No one else requires a pointer to BrowserShortcutLauncherItemController.

[simonhong_, 2013-09-06 00:40:01]

Dear stefan,
Please check with patch set #3.

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:75:
GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
On 2013/09/05 23:30:13, Mr4D wrote:
> You misunderstood - I was wondering why UpdateAppState(..) isn't at the same
> time calling the UpdateBrowserItemStatus. This was maybe a bad example, but
when
> you look into line 153, 160, 167, 175, 186 you will notice that the two calls
> always come in pairs...

Ah.. your concern is why UpdateAppState() and UpdateBrowserItemStautus() aren't
always called both.

First, they are called in pair when an event is occurred from TabStripModel.
In line 75, 83, If browser has active web contents, both are called in pair.
I think 105 is not needed. When browser is created browser item and app item
status can be updated by TabInsertedAt().
In line 115, When browser is removed, app's item status is already updated by
TabDetachedAt(). So in here, only browser item state update is needed.

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:208:
BrowserStatusMonitor::GetBrowserShortcutLauncherItemController() {
On 2013/09/05 23:30:13, Mr4D wrote:
> You have misunderstood:
> 
> void BrowserStatusMonitor::UpdateBrowserItemStatus() {
>  
>
launcher_controller_->GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
> }
> 
> No one else requires a pointer to BrowserShortcutLauncherItemController.

Ah.. I made ugly code..
Done.

[Mr4D (OOO till 08-26), 2013-09-06 16:11:12]

Please have another look!

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:75:
GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
For line 75 / 83: An if .. else .. could resolve that.

For all others: You can remove them by adding the line after line 204 and
include a bracket around 190, throwing in another call). Then rename the
function UpdateAppState to UpdateAppAndBrowserState.

Thanks for removing 105 again! (somehow I didn't realize why I didn't have it
there before).

https://codereview.chromium.org/23534022/diff/22001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_status_monitor.h (right):

https://codereview.chromium.org/23534022/diff/22001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.h:85: // Update browser
item status.
The comment is the same as the function name. What about:

// A shortcut to call the BrowserShortcutLauncherItemController's
UpdateBrowserItemStatus.

[simonhong_, 2013-09-06 18:52:57]

Dear stefan,
Please check one-more time!

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23534022/diff/1/chrome/browser/ui/ash/launche...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:75:
GetBrowserShortcutLauncherItemController()->UpdateBrowserItemStatus();
On 2013/09/06 16:11:12, Mr4D wrote:
> For line 75 / 83: An if .. else .. could resolve that.
> 
> For all others: You can remove them by adding the line after line 204 and
> include a bracket around 190, throwing in another call). Then rename the
> function UpdateAppState to UpdateAppAndBrowserState.
> 
> Thanks for removing 105 again! (somehow I didn't realize why I didn't have it
> there before).
> 

Done.

https://codereview.chromium.org/23534022/diff/22001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_status_monitor.h (right):

https://codereview.chromium.org/23534022/diff/22001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.h:85: // Update browser
item status.
On 2013/09/06 16:11:12, Mr4D wrote:
> The comment is the same as the function name. What about:
> 
> // A shortcut to call the BrowserShortcutLauncherItemController's
> UpdateBrowserItemStatus.

Done.

[Mr4D (OOO till 08-26), 2013-09-06 20:57:42]

lgtm

[commit-bot: I haz the power, 2013-09-06 20:58:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simon.hong81@gmail.com/23534022/28001

[commit-bot: I haz the power, 2013-09-07 00:07:40]

Change committed as 221823