TimelinePanel: introduce StackView and use it instead of _timelineMemorySplitter

TimelinePanel: introduce StackView and use it instead of _timelineMemorySplitter

move recordsView construction code into a private method

Drive by fix: remove _popoverHelper and related code. We don't use it any more.

BUG=
R=aandrey@chromium.org, yurys@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165121

[yurys, 2014-01-14 15:15:44]

StackView.js file is missing

[pfeldman, 2014-01-14 16:32:33]

https://codereview.chromium.org/137853005/diff/50001/Source/devtools/front_en...
File Source/devtools/front_end/StackView.js (right):

https://codereview.chromium.org/137853005/diff/50001/Source/devtools/front_en...
Source/devtools/front_end/StackView.js:33: WebInspector.StackView = function()
isVertical should be here.

https://codereview.chromium.org/137853005/diff/50001/Source/devtools/front_en...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/137853005/diff/50001/Source/devtools/front_en...
Source/devtools/front_end/TimelinePanel.js:42: importScript("StackView.js");
move to UI component, bake into core (inspector.html)

[loislo, 2014-01-14 16:38:10]

https://codereview.chromium.org/137853005/diff/50001/Source/devtools/front_en...
File Source/devtools/front_end/StackView.js (right):

https://codereview.chromium.org/137853005/diff/50001/Source/devtools/front_en...
Source/devtools/front_end/StackView.js:33: WebInspector.StackView = function()
On 2014/01/14 16:32:33, pfeldman wrote:
> isVertical should be here.

Done.

https://codereview.chromium.org/137853005/diff/50001/Source/devtools/front_en...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/137853005/diff/50001/Source/devtools/front_en...
Source/devtools/front_end/TimelinePanel.js:42: importScript("StackView.js");
On 2014/01/14 16:32:33, pfeldman wrote:
> move to UI component, bake into core (inspector.html)

Done.

[aandrey, 2014-01-14 20:06:42]

Mind adding a screenshot if the UI changed? If the UI will not be changed,
please indicate this in the patch description.

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
File Source/devtools/front_end/StackView.js (right):

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/StackView.js:34: WebInspector.StackView =
function(isVertical)
nit: bool flag to an enum? I had to move back to see what "new
WebInspector.StackView(false)" does.

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
File Source/devtools/front_end/TimelineView.js (right):

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/TimelineView.js:172: _createRecordsView: function()
@return jsdoc

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/TimelineView.js:208: this._mainThreadTasks = /** @type
{!Array.<!TimelineAgent.TimelineEvent>} */ ([]);
nit: make it type def instead of type cast:

  /** @type {!Array.<!TimelineAgent.TimelineEvent>} */
  this._mainThreadTasks = [];

[loislo, 2014-01-15 07:48:51]

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
File Source/devtools/front_end/StackView.js (right):

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/StackView.js:34: WebInspector.StackView =
function(isVertical)
On 2014/01/14 20:06:43, aandrey wrote:
> nit: bool flag to an enum? I had to move back to see what "new
> WebInspector.StackView(false)" does.

originaly it was bool and it is the only parameter so it is not a problem yet.

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
File Source/devtools/front_end/TimelineView.js (right):

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/TimelineView.js:172: _createRecordsView: function()
On 2014/01/14 20:06:43, aandrey wrote:
> @return jsdoc

Done.

https://codereview.chromium.org/137853005/diff/140001/Source/devtools/front_e...
Source/devtools/front_end/TimelineView.js:208: this._mainThreadTasks = /** @type
{!Array.<!TimelineAgent.TimelineEvent>} */ ([]);
On 2014/01/14 20:06:43, aandrey wrote:
> nit: make it type def instead of type cast:
> 
>   /** @type {!Array.<!TimelineAgent.TimelineEvent>} */
>   this._mainThreadTasks = [];

I just moved the code.
Also I see no difference from practical point of view.
The same construction is used in many places.

So if you have strong pros then I can do that in a separate patch in all
devtools code.

[aandrey, 2014-01-15 08:11:22]

> On 2014/01/14 20:06:43, aandrey wrote:
> > nit: make it type def instead of type cast:
> > 
> >   /** @type {!Array.<!TimelineAgent.TimelineEvent>} */
> >   this._mainThreadTasks = [];
> 
> I just moved the code.
> Also I see no difference from practical point of view.
> The same construction is used in many places.
> 
> So if you have strong pros then I can do that in a separate patch in all
> devtools code.

I may be wrong, but IMO jscompiler may not alter the type after the cast, and
leave it as it was before the case, i.e. @type {?} == no type-checking.
If this is the case, these kind of casts are just no-op.
The type def syntax makes jscompiler do its work on type checking.

If you care, try to play with the compiler and insert a wrong argument into
this._mainThreadTasks. If the compiler is silent, we should use type def syntax.

[loislo, 2014-01-15 09:08:30]

On 2014/01/15 08:11:22, aandrey wrote:
> > On 2014/01/14 20:06:43, aandrey wrote:
> > > nit: make it type def instead of type cast:
> > > 
> > >   /** @type {!Array.<!TimelineAgent.TimelineEvent>} */
> > >   this._mainThreadTasks = [];
> > 
> > I just moved the code.
> > Also I see no difference from practical point of view.
> > The same construction is used in many places.
> > 
> > So if you have strong pros then I can do that in a separate patch in all
> > devtools code.
> 
> I may be wrong, but IMO jscompiler may not alter the type after the cast, and
> leave it as it was before the case, i.e. @type {?} == no type-checking.
> If this is the case, these kind of casts are just no-op.
> The type def syntax makes jscompiler do its work on type checking.
> 
> If you care, try to play with the compiler and insert a wrong argument into
> this._mainThreadTasks. If the compiler is silent, we should use type def
syntax.

I tested that with the next snippet

        /**
         * @param {!Array.<!TimelineAgent.TimelineEvent>} t
         */
        function test(t) { }
        
        this._mainThreadTasks = /** @type {!Array.<!string>} */ ([]);
        test(this._mainThreadTasks);

and compiler generated an error:
WARNING - actual parameter 1 of test does not match formal parameter
found   : Array.<string>
required: Array.<{children: (Array.<?>|undefined), counters:
(undefined|{documents: (number|undefined), jsEventListeners: (number|undefined),
jsHeapSizeUsed: (number|undefined), nodes: (number|undefined), ...}),

So the compiler assigned an inferred type to a newly defined variable from the
assignment operator ant this type may could be changed
by the followup assignment operator.

So the snippet
    this._mainThreadTasks = /** @type {!Array.<!string>} */ ([]);
    this._mainThreadTasks = /** @type {!Array.<!TimelineAgent.TimelineEvent>} */
([]);
is valid.

The suggested schema has more strong typing support.

        /** @type {!Array.<!string>} */
        this._mainThreadTasks =  ([]);
        this._mainThreadTasks = /** @type
{!Array.<!TimelineAgent.TimelineEvent>} */ ([]);

is not valid. The compiler generates warning on the second assignment operator.

[aandrey, 2014-01-15 10:49:21]

On 2014/01/15 09:08:30, loislo wrote:
> On 2014/01/15 08:11:22, aandrey wrote:
> > > On 2014/01/14 20:06:43, aandrey wrote:
> > > > nit: make it type def instead of type cast:
> > > > 
> > > >   /** @type {!Array.<!TimelineAgent.TimelineEvent>} */
> > > >   this._mainThreadTasks = [];
> > > 
> > > I just moved the code.
> > > Also I see no difference from practical point of view.
> > > The same construction is used in many places.
> > > 
> > > So if you have strong pros then I can do that in a separate patch in all
> > > devtools code.
> > 
> > I may be wrong, but IMO jscompiler may not alter the type after the cast,
and
> > leave it as it was before the case, i.e. @type {?} == no type-checking.
> > If this is the case, these kind of casts are just no-op.
> > The type def syntax makes jscompiler do its work on type checking.
> > 
> > If you care, try to play with the compiler and insert a wrong argument into
> > this._mainThreadTasks. If the compiler is silent, we should use type def
> syntax.
> 
> I tested that with the next snippet
> 
>         /**
>          * @param {!Array.<!TimelineAgent.TimelineEvent>} t
>          */
>         function test(t) { }
>         
>         this._mainThreadTasks = /** @type {!Array.<!string>} */ ([]);
>         test(this._mainThreadTasks);
> 
> and compiler generated an error:
> WARNING - actual parameter 1 of test does not match formal parameter
> found   : Array.<string>
> required: Array.<{children: (Array.<?>|undefined), counters:
> (undefined|{documents: (number|undefined), jsEventListeners:
(number|undefined),
> jsHeapSizeUsed: (number|undefined), nodes: (number|undefined), ...}),
> 
> So the compiler assigned an inferred type to a newly defined variable from the
> assignment operator ant this type may could be changed
> by the followup assignment operator.
> 
> So the snippet
>     this._mainThreadTasks = /** @type {!Array.<!string>} */ ([]);
>     this._mainThreadTasks = /** @type {!Array.<!TimelineAgent.TimelineEvent>}
*/
> ([]);
> is valid.
> 
> The suggested schema has more strong typing support.
> 
>         /** @type {!Array.<!string>} */
>         this._mainThreadTasks =  ([]);
>         this._mainThreadTasks = /** @type
> {!Array.<!TimelineAgent.TimelineEvent>} */ ([]);
> 
> is not valid. The compiler generates warning on the second assignment
operator.

Thanks. But I meant:

this._mainThreadTasks = /** @type {!Array.<!TimelineAgent.TimelineEvent>} */
([]);
this._mainThreadTasks.push("str");

vs.

/** @type {!Array.<!TimelineAgent.TimelineEvent>} */
this._mainThreadTasks = [];
this._mainThreadTasks.push("str");

[pfeldman, 2014-01-15 11:44:36]

https://codereview.chromium.org/137853005/diff/230001/Source/devtools/front_e...
File Source/devtools/front_end/TimelineView.js (left):

https://codereview.chromium.org/137853005/diff/230001/Source/devtools/front_e...
Source/devtools/front_end/TimelineView.js:1215: _closeRecordDetails: function()
Why did this go?

[loislo, 2014-01-15 13:05:22]

On 2014/01/15 11:44:36, pfeldman wrote:
>
https://codereview.chromium.org/137853005/diff/230001/Source/devtools/front_e...
> File Source/devtools/front_end/TimelineView.js (left):
> 
>
https://codereview.chromium.org/137853005/diff/230001/Source/devtools/front_e...
> Source/devtools/front_end/TimelineView.js:1215: _closeRecordDetails:
function()
> Why did this go?

we don't use popover any more so we don't need this function

[aandrey, 2014-01-15 13:08:44]

lgtm

[yurys, 2014-01-15 13:43:03]

lgtm

https://codereview.chromium.org/137853005/diff/230001/Source/devtools/scripts...
File Source/devtools/scripts/frontend_modules.json (right):

https://codereview.chromium.org/137853005/diff/230001/Source/devtools/scripts...
Source/devtools/scripts/frontend_modules.json:263: "StackView.js",
Keep files sorted?

[loislo, 2014-01-15 13:52:11]

Committed patchset #6 manually as r165121 (presubmit successful).

[alph, 2014-01-15 13:55:38]

https://codereview.chromium.org/137853005/diff/230001/Source/devtools/front_e...
File Source/devtools/front_end/StackView.js (right):

https://codereview.chromium.org/137853005/diff/230001/Source/devtools/front_e...
Source/devtools/front_end/StackView.js:48: appendView: function(view,
sidebarSizeSettingName, defaultSidebarWidth, defaultSidebarHeight)
It's not clear which one is a sidebar(s) in StackView. Could there be better
names for the arguments?

[pfeldman, 2014-01-15 14:04:09]

A revert of this CL has been created in
https://codereview.chromium.org/139413002/ by pfeldman@chromium.org.

The reason for reverting is: popover dismiss code was removed.