Reduce calling count of UpdateAppState()

Reduce calling count of UpdateAppState()

When web page is loading, a lot of TabChangedAt() is called. That means a
lot of UpdateAppState() is called, too.
For example, loading google drive site causes almost more than 70 calling
of TabChangedAt().
To replace TabChangedAt(), WebContentsObserver is used.

TabDetachedAt() is replaced with TabClosingAt().
When tab is detached, there are two cases.
The one case is tab is attached to other tab strip model. In this case, 
its LauncherItem is updated by ActiveTabChanged().
The other case is tab is closed. In this case TabClosingAt() can handle LauncherItem status.
TabReplacedAt() is only used when tab is discarded when low memory condition is occurred.
In this case, updating the launcher item status is not good because user don't need to know
about discarding a tab. Also it is reloaded again when user selects discarded tab.

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

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

[simonhong_, 2013-09-10 08:49:51]

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

[Mr4D (OOO till 08-26), 2013-09-10 21:57:00]

Hmm. My first reaction was: Why do you not let the Monitor keep a map of content
<-> URL's and compare if there was a change?

I can see why you are doing this - and I am sure that it will work, but I will
defer with this to sky since I am not sure if adding a new notification is a
good idea.

[simonhong_, 2013-09-10 22:36:21]

On 2013/09/10 21:57:00, Mr4D wrote:
> Hmm. My first reaction was: Why do you not let the Monitor keep a map of
content
> <-> URL's and compare if there was a change?
> 
> I can see why you are doing this - and I am sure that it will work, but I will
> defer with this to sky since I am not sure if adding a new notification is a
> good idea.

Dear sky,
With this new flag, we can update LauncherItem more easily and efficiently.
Please take a look.
Thank you.
Simon.

[sky, 2013-09-11 02:09:58]

https://codereview.chromium.org/23708028/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/23708028/diff/1/chrome/browser/ui/browser.cc#...
chrome/browser/ui/browser.cc:1920: TabStripModelObserver::URL_CHANGING_ONLY);
Your usage here is not entirely correct so that URL_CHANGING_ONLY becomes
misleading and error prone.

If BrowserStatusMonitor only cares about the url of the active tab it should be
able to monitor that directly.

[simonhong_, 2013-09-12 17:24:14]

Dear stefan,
I monitored active tab's WebContents instead of adding new notification.
Also some functions (TabChangedAt(), TabReplacedAt()) are removed. The reason of
this removing is written in this CL's description.
Please check again.

[simonhong_, 2013-09-13 08:57:30]

On 2013/09/12 17:24:14, Simon YoungKi Hong wrote:
> Dear stefan,
> I monitored active tab's WebContents instead of adding new notification.
> Also some functions (TabChangedAt(), TabReplacedAt()) are removed. The reason
of
> this removing is written in this CL's description.
> Please check again.

Dear stefan,
I current version of this CL is not perfect to request review.
Please ignore review request of this CL.
I'll send again when CL is prepared well soon.
Sorry to disturb.
Simon.

[Mr4D (OOO till 08-26), 2013-09-13 14:35:17]

A few more comments - but looks better then the first version!

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:39: // Don't need to
update item state when page is reloaded.
What about:

No need to update the item state when the page gets reloaded.

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:40: if (reload_type ==
content::NavigationController::NO_RELOAD) {
No {} here.

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:188: if
(webcontents_to_observer_map_.find(old_contents) !=
You might want to put this deletion code into a function and call it from
ActiveTabChanged as well as from TabClosingAt.

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:217: int index) {
Is this really the same event? Did you try to drag a tab from one browser to
another and this was called?

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

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.h:80: // a navigation
changing to BrowserStatusMonitor.
This class monitors the WebContent of the active tab and notifies a navigation
to the BrowserStatusMonitor.

[simonhong_, 2013-09-15 17:27:07]

Dear stefan,
I refactored previous patch.
Please check again!

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:39: // Don't need to
update item state when page is reloaded.
On 2013/09/13 14:35:18, Mr4D wrote:
> What about:
> 
> No need to update the item state when the page gets reloaded.

I removed this method and only DidNavigateMainFrame() is used.
When NavigateToPendingEntry() is called we can't update apps state because
webcontents is not updated to new url at this time.

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:40: if (reload_type ==
content::NavigationController::NO_RELOAD) {
On 2013/09/13 14:35:18, Mr4D wrote:
> No {} here.

Done.

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:188: if
(webcontents_to_observer_map_.find(old_contents) !=
On 2013/09/13 14:35:18, Mr4D wrote:
> You might want to put this deletion code into a function and call it from
> ActiveTabChanged as well as from TabClosingAt.

I changed to observe all web contents.
Inactive webcontent can also change its url.
So only deletion code is used at TabClosingAt().

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:217: int index) {
On 2013/09/13 14:35:18, Mr4D wrote:
> Is this really the same event? Did you try to drag a tab from one browser to
> another and this was called?

Not same event.

When drag and drop a tab from one browser(A) to another browser(B),
TabClosingAt() is not called. Followings are the call sequence.
1) TabDetachedAt() called at A.
2) ActiveTabChanged() called at A.
3) TabInsertedAt() called at C(temp browser).
4) ActiveTabChanged() called at C.
5) TabDetachedAt() called at C.
6) TabInsertedAt() called at B.
7) ActiveTabChanged() called at B.
During the drag and drop same WebContents is used.
So we don't need to destroy observer here.

TabClosingAt () is only called when tab is closed.
So observer is destroyd when TabClosingAt() is called.

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

https://codereview.chromium.org/23708028/diff/11001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.h:80: // a navigation
changing to BrowserStatusMonitor.
On 2013/09/13 14:35:18, Mr4D wrote:
> This class monitors the WebContent of the active tab and notifies a navigation
> to the BrowserStatusMonitor.

Done.

[Mr4D (OOO till 08-26), 2013-09-18 01:43:49]

Thanks for trying to reduce calls! 

Please have a look at my comments!

Note: We are getting very close to M-31 and should possibly not check in any non
required refactoring CL's.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:127: if (contents) {
since contents was already set in 114 - you could come here even though browser
is NULL.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:134: if (contents)
I am not sure which contents you are referring to here? I suppose either content
from 114 or contents from 126?  It cannot be both.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:204: // Use
|new_contents|. |old_contents| could be NULL.
Just checking: Is there a guarantee that new_contents is not NULL? Maybe you
want to add a DCHECK?

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:215: if (browser &&
maybe you want to test also for old_contents == NULL here?

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:237: 
same what I commented on in 264 is valid here. You might have a browser with an
app running. Then you insert a new content tab which will raise the browser item
from APP_STATE_INACTIVE to something more active.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:245: // Because of this
reason, |APP_STATE_INACTIVE| is set to all tab inserting.
Instead of the comment above: what about 
// An inserted tab is not active - ActiveTabChanged will be called to activate.
We initialize therefore with |APP_STATE_INACTIVE|.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:251: // be by other
tab's state change.
And why should any tab's state change if it only get inserted without getting
activated?

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:264:
ChromeLauncherController::APP_STATE_REMOVED);
You can remove a browser tab which is not an app and all which is then left is
an app. When that happens the browserItemState should go inactive and thus the
Browser Item State needs updating.

Why did you remove it?

[simonhong_, 2013-09-18 13:38:38]

Thank you for review!
I uploaded new patch set and added some comments.
Please check again.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:127: if (contents) {
On 2013/09/18 01:43:49, Mr4D wrote:
> since contents was already set in 114 - you could come here even though
browser
> is NULL.

Done.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:134: if (contents)
On 2013/09/18 01:43:49, Mr4D wrote:
> I am not sure which contents you are referring to here? I suppose either
content
> from 114 or contents from 126?  It cannot be both.

My purpose was updating browser item state only if one of them is not NULL or
both are not NULL.
Fixed.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:204: // Use
|new_contents|. |old_contents| could be NULL.
On 2013/09/18 01:43:49, Mr4D wrote:
> Just checking: Is there a guarantee that new_contents is not NULL? Maybe you
> want to add a DCHECK?

I checked that |new_contents| is set to WebContents of current active index in
TabStripModel::NotifyIfActiveTabChanged().
So I think it will be always not NULL.
DCHECK() is added instead of if ().

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:215: if (browser &&
On 2013/09/18 01:43:49, Mr4D wrote:
> maybe you want to test also for old_contents == NULL here?

Done.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:237: 
On 2013/09/18 01:43:49, Mr4D wrote:
> same what I commented on in 264 is valid here. You might have a browser with
an
> app running. Then you insert a new content tab which will raise the browser
item
> from APP_STATE_INACTIVE to something more active.

When an app tab is running, browser item state is already running state.
When a new tab inserted into this browser, its state is updated to running
state. If this tab becomes active, the new tab's item and browser item state
will be updated by ActiveTabChanged().

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:245: // Because of this
reason, |APP_STATE_INACTIVE| is set to all tab inserting.
On 2013/09/18 01:43:49, Mr4D wrote:
> Instead of the comment above: what about 
> // An inserted tab is not active - ActiveTabChanged will be called to
activate.
> We initialize therefore with |APP_STATE_INACTIVE|.

Done.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:251: // be by other
tab's state change.
On 2013/09/18 01:43:49, Mr4D wrote:
> And why should any tab's state change if it only get inserted without getting
> activated?
If we don't update the state of inserted tab, all inactive tab's item state is
closed state on startup. They should be updated to running state.
I found one case on startup phase that tab is inserted and not become an active
tab.
When user select the startup option("Continue where I left off" or "Open a
specific page or set of pages"), many tabs can be created on startup.
In this case ActiveTabChanged() is called only once just for active tab with
old_contents(NULL). That means if we update all inactive tabs state when it is
inserted, its state will be closed state.

https://codereview.chromium.org/23708028/diff/21001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:264:
ChromeLauncherController::APP_STATE_REMOVED);
On 2013/09/18 01:43:49, Mr4D wrote:
> You can remove a browser tab which is not an app and all which is then left is
> an app. When that happens the browserItemState should go inactive and thus the
> Browser Item State needs updating.
> 
> Why did you remove it?

When a tab is closing, ActiveTabChanged() is also called after TabClosingAt() if
it was not the only one tab in the tab strip.
Browser item and next active app tab state can be updated by ActiveTabChanged().
Also when a tab is the only one, browser item state can be updated by
OnBrowserRemoved().

[simonhong_, 2013-09-30 14:02:09]

Dear stefan,
I added additional unittests for tab drag and drop.
Please check again.

[Mr4D (OOO till 08-26), 2013-10-01 03:20:33]

A few more comments. Please have a look!

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:206:
DCHECK(new_contents);
Hmm. What happens when a browser goes away? As far as I know this is done by
deleting tab by tab until all are gone - in which case new_contents could also
be NULL. Can you verify that this case does not happen?

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:271: void
BrowserStatusMonitor::CreateWebContentsObserver(
That sounds odd. This should be something like "AddWebContentsObserver".

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:838: // For empty
|app_id| tab, its state is udpated by UpdateBrowserItemState().
What about:

UpdateBrowserItemState will update the state when no application is associated
with the tab.

[simonhong_, 2013-10-01 05:10:43]

Dear stefan,
Please check again!

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/browser_status_monitor.cc (right):

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:206:
DCHECK(new_contents);
On 2013/10/01 03:20:33, Mr4D wrote:
> Hmm. What happens when a browser goes away? As far as I know this is done by
> deleting tab by tab until all are gone - in which case new_contents could also
> be NULL. Can you verify that this case does not happen?

I checked that only TabClosingAt() was called when last tab is deleted.
Also,I added CloseAllTabs() in the TabDragAndDrop test in browsertests.

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/browser_status_monitor.cc:271: void
BrowserStatusMonitor::CreateWebContentsObserver(
On 2013/10/01 03:20:33, Mr4D wrote:
> That sounds odd. This should be something like "AddWebContentsObserver".

Done.

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc (right):

https://codereview.chromium.org/23708028/diff/33001/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc:838: // For empty
|app_id| tab, its state is udpated by UpdateBrowserItemState().
On 2013/10/01 03:20:33, Mr4D wrote:
> What about:
> 
> UpdateBrowserItemState will update the state when no application is associated
> with the tab.

Done.

[Mr4D (OOO till 08-26), 2013-10-02 03:32:00]

lgtm

[commit-bot: I haz the power, 2013-10-02 04:24:14]

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

[commit-bot: I haz the power, 2013-10-02 06:54:30]

Change committed as 226406