WebUI TaskManager: Optimize initialization taskmanager and loading scripts.

WebUI TaskManager: Optimize initialization taskmanager and loading scripts.

BUG=104136
TEST=manual on linux


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113772

[yoshiki, 2011-11-28 17:20:39]

Could you take a look it?

[mazda, 2011-11-29 04:25:59]

As we talked offline, it might be better to avoid polling if possible.

[yoshiki, 2011-11-30 08:59:53]

Thank you for comments. I've omitted polling. Could you take a look this?

On 2011/11/29 04:25:59, mazda wrote:
> As we talked offline, it might be better to avoid polling if possible.

[mazda, 2011-11-30 12:05:54]

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
File chrome/browser/resources/task_manager/includes.js (right):

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:15: var isLoading =
script.length;
It's better to rename the variable to something that represents number.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:24: // Waits for initialize of
task manager.
initialize -> initialization

Please add a comment on why we load scripts every 100 ms.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:28: var src = prefix + 'js/' +
script[i];
It's better to assign the value directly to s.src rather than making a variable.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:31: isLoading--; if(isLoading
== 0) taskmanager.delayedInitialize();
Please add line breaks.
Also need a space after 'if'.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:32: console.log("aaa" +
e.target.src);
Remove this.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:38: console.log("hogehoge" +
src);
Remove this.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:41:
window.setTimeout(loadDelayedScripts, 100);
Please make a constant variable for 100.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:44:
window.setTimeout(loadDelayedScripts, 100);
Ditto.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/main.js:380: if
(this.isFinishedInitDelayed_) {
Is it possible to reach here without this.isFinishedInitDelayed_ being truef?

[yoshiki, 2011-12-01 16:18:25]

I'm very sorry that I've submitted the temporary code as Patch Set #2 and #3. I
sent a new code (Patch Set #4). Could you take a look at it again? Sorry for
unnecessary double work.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
File chrome/browser/resources/task_manager/includes.js (right):

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/includes.js:24: // Waits for initialize of
task manager.
On 2011/11/30 12:05:54, mazda wrote:
> initialize -> initialization
> 
> Please add a comment on why we load scripts every 100 ms.

Done.

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/8713016/diff/7014/chrome/browser/resources/tas...
chrome/browser/resources/task_manager/main.js:380: if
(this.isFinishedInitDelayed_) {
Delayed scripts are loaded with 'defer' property, so that the loading and
execution of them would be delayed. Loading and execution are takes ~50ms, and
updating tasks comes ~100ms after the initialization, at least in my
environment. I think it is possible to update tasks before the loading and
execution were finished.

On 2011/11/30 12:05:54, mazda wrote:
> Is it possible to reach here without this.isFinishedInitDelayed_ being truef?

[mazda, 2011-12-05 03:42:11]

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/includes.js (right):

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/includes.js:5: // This script loads
additional scripts after initialize of task manager.
initialize -> initialization

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/includes.js:26: var isLoading =
script.length;
Please rename |isLoading| to something that represents number.

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/main.js:382: if
(this.isFinishedInitDelayed_) {
Is it OK to reach here when this.isFinishedInitDelaed_ is false?

[yoshiki, 2011-12-05 08:38:15]

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/includes.js (right):

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/includes.js:5: // This script loads
additional scripts after initialize of task manager.
On 2011/12/05 03:42:11, mazda wrote:
> initialize -> initialization

Done.

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/includes.js:26: var isLoading =
script.length;
On 2011/12/05 03:42:11, mazda wrote:
> Please rename |isLoading| to something that represents number.

Done.

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/main.js:382: if
(this.isFinishedInitDelayed_) {
It's ok. This block adds context menu handler to label. If it is skipped, only
context menu does not work.

On 2011/12/05 03:42:11, mazda wrote:
> Is it OK to reach here when this.isFinishedInitDelaed_ is false?

[mazda, 2011-12-06 09:58:18]

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/main.js:382: if
(this.isFinishedInitDelayed_) {
On 2011/12/05 08:38:15, yoshiki wrote:
> It's ok. This block adds context menu handler to label. If it is skipped, only
> context menu does not work.
> 
> On 2011/12/05 03:42:11, mazda wrote:
> > Is it OK to reach here when this.isFinishedInitDelaed_ is false?
> 

I think it's undesirable. 
How about storing the labels and initializing them in delayedInitialize if
|this.isFinishedInitDelayed_| is false?

[yoshiki, 2011-12-07 16:01:09]

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/8713016/diff/19001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/main.js:382: if
(this.isFinishedInitDelayed_) {
On 2011/12/06 09:58:18, mazda wrote:
> On 2011/12/05 08:38:15, yoshiki wrote:
> > It's ok. This block adds context menu handler to label. If it is skipped,
only
> > context menu does not work.
> > 
> > On 2011/12/05 03:42:11, mazda wrote:
> > > Is it OK to reach here when this.isFinishedInitDelaed_ is false?
> > 
> 
> I think it's undesirable. 
> How about storing the labels and initializing them in delayedInitialize if
> |this.isFinishedInitDelayed_| is false?

Done.

[yoshiki, 2011-12-07 16:01:26]

PATL

[mazda, 2011-12-08 09:14:29]

lgtm

[commit-bot: I haz the power, 2011-12-08 09:42:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/8713016/28001

[commit-bot: I haz the power, 2011-12-08 09:42:22]

Presubmit check for 8713016-28001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/task_manager/includes.js,chrome/browser/resources/task_manager/main.html,chrome/browser/resources/task_manager/main.js

Presubmit checks took 1.1s to calculate.

[yoshiki, 2011-12-08 09:46:14]

jhawkins: PATL

[James Hawkins, 2011-12-08 18:14:09]

LGTM with two nits.

http://codereview.chromium.org/8713016/diff/28001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/includes.js (right):

http://codereview.chromium.org/8713016/diff/28001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/includes.js:30: for (var i = 0; i <
script.length; ++i) {
i++

http://codereview.chromium.org/8713016/diff/28001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/8713016/diff/28001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/main.js:227: while
(this.delayedInitLabels_.length > 0) {
No braces for one-link blocks.

[yoshiki, 2011-12-09 05:51:11]

Thanks!

http://codereview.chromium.org/8713016/diff/28001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/includes.js (right):

http://codereview.chromium.org/8713016/diff/28001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/includes.js:30: for (var i = 0; i <
script.length; ++i) {
Coding style says preincrement is better.

On 2011/12/08 18:14:09, James Hawkins wrote:
> i++

http://codereview.chromium.org/8713016/diff/28001/chrome/browser/resources/ta...
File chrome/browser/resources/task_manager/main.js (right):

http://codereview.chromium.org/8713016/diff/28001/chrome/browser/resources/ta...
chrome/browser/resources/task_manager/main.js:227: while
(this.delayedInitLabels_.length > 0) {
On 2011/12/08 18:14:09, James Hawkins wrote:
> No braces for one-link blocks.

Done.

[commit-bot: I haz the power, 2011-12-09 05:51:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/8713016/34004

[commit-bot: I haz the power, 2011-12-09 07:15:15]

Change committed as 113772