[Devtools] Replace "Stacks" with "Causes"

[Devtools] Replace "Stacks" with "Causes"

This patch switches to "Causes" for explaining timeline records. The
major change is "Stacks" have been renamed "Causes". This is prepwork
for two upcoming projects: invalidation tracking which will fill out many
more causes, and javascript samples which are easily confused with
the "stack" term. If the causes checkbox is not checked, no initiator
or stack information will be shown.

Some minor rewording of the causes has also been done. For example,
the stack for a layout now reads "First layout invalidation" instead of
"Layout invalidated".

A screenshot is available at http://pr.gg/justcause3.png

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

[pdr., 2014-10-05 04:05:37]

pdr@chromium.org changed reviewers:
+ alph@google.com, caseq@chromium.org, yurys@chromium.org

[caseq, 2014-10-06 12:47:48]

https://codereview.chromium.org/631573002/diff/20001/LayoutTests/inspector/tr...
File LayoutTests/inspector/tracing/timeline-event-causes.html (right):

https://codereview.chromium.org/631573002/diff/20001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:6: function test() {
style: here and below, { => next line.

https://codereview.chromium.org/631573002/diff/20001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:15:
setTimeout(function() {
just setTimeout(callback, 0) perhaps?

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1019:
causesView.setContentGenerator(function(event, callback) {
This looks like a bit too much inversion of control, do we need it? I wonder if
we can either inline this into the causes view or perhaps just call
generateCausesContent and pass the result down to the view?

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1143: return
this._captureCausesSetting && this._captureCausesSetting.get()
does this._captureCausesSetting ever evaluate to false?

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1206: // FIXME: We should
check (this._lastUserSelectedTabId) here so new tabs
I think this logic is still producing the effect originally intended, although
it may indeed look weird: if the user has made an explicit choice and the
selected tab is available [even as we switch to a different event], we honor
user choice. Otherwise, show the last tab as we expect it to be more useful than
generic one.

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1547: _noCauseFoundContent:
function()
nit: since it's going to be always the same, just inline creation in the
constructor?

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:708:
callback(contentHelper.element);
We always invoke callback synchronously -- let's just return the element
instead?

[pdr., 2014-10-07 04:36:13]

Thanks for the review! Much simpler now. PTAL

https://codereview.chromium.org/631573002/diff/20001/LayoutTests/inspector/tr...
File LayoutTests/inspector/tracing/timeline-event-causes.html (right):

https://codereview.chromium.org/631573002/diff/20001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:6: function test() {
On 2014/10/06 at 12:47:47, caseq wrote:
> style: here and below, { => next line.

Done.

https://codereview.chromium.org/631573002/diff/20001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:15:
setTimeout(function() {
On 2014/10/06 at 12:47:47, caseq wrote:
> just setTimeout(callback, 0) perhaps?

Done.

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1019:
causesView.setContentGenerator(function(event, callback) {
On 2014/10/06 at 12:47:48, caseq wrote:
> This looks like a bit too much inversion of control, do we need it? I wonder
if we can either inline this into the causes view or perhaps just call
generateCausesContent and pass the result down to the view?

Done.

In the future generateCausesContent may be expensive if there are many causes so
I was doing that work asynchronously. This was overengineering for the causes
currently listed, and may be in the future as well, so I've removed this crazzy
and now we just call generateContent directly.

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1143: return
this._captureCausesSetting && this._captureCausesSetting.get()
On 2014/10/06 at 12:47:48, caseq wrote:
> does this._captureCausesSetting ever evaluate to false?

I was being too cautious. I don't think we can get here without the setting so
I've updated this.

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1206: // FIXME: We should
check (this._lastUserSelectedTabId) here so new tabs
On 2014/10/06 at 12:47:48, caseq wrote:
> I think this logic is still producing the effect originally intended, although
it may indeed look weird: if the user has made an explicit choice and the
selected tab is available [even as we switch to a different event], we honor
user choice. Otherwise, show the last tab as we expect it to be more useful than
generic one.

I've pulled this out into https://codereview.chromium.org/627413002.

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1547: _noCauseFoundContent:
function()
On 2014/10/06 at 12:47:48, caseq wrote:
> nit: since it's going to be always the same, just inline creation in the
constructor?

I wanted to keep this as light as possible, as it should never be shown. Before
showing the "causes" tab we make sure that there are causes, so this error
message is just for catching errors. For your comment above about
generateContent being sort of crazy, I've refactored how this works so
_noCauseFoundContent is now inlined.

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/631573002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:708:
callback(contentHelper.element);
On 2014/10/06 at 12:47:48, caseq wrote:
> We always invoke callback synchronously -- let's just return the element
instead?

Good catch. Done.

[paulirish, 2014-10-07 05:10:52]

I don't like moving the callstacks off to a secondary tab. It doesn't seem like
we have enough detail per record that we need to start subdividing record
entries into pieces yet. 
Since details is default tab, this means adding another click not just to hunt
down where this invalidation happened, but adding a click before the user can
even see where it's from.

The callstack is probably the most important information in Details view and
especially now that it's being upgraded, we should have it front and center for
the users.

[pdr., 2014-10-07 05:24:25]

On 2014/10/07 at 05:10:52, paulirish wrote:
> I don't like moving the callstacks off to a secondary tab. It doesn't seem
like we have enough detail per record that we need to start subdividing record
entries into pieces yet. 
> Since details is default tab, this means adding another click not just to hunt
down where this invalidation happened, but adding a click before the user can
even see where it's from.
> 
> The callstack is probably the most important information in Details view and
especially now that it's being upgraded, we should have it front and center for
the users.

We're going to run out of space with the invalidations tracking project. What do
you think about making the causes tab show up automatically as default instead
of the details tab?

[yurys, 2014-10-07 05:35:25]

yurys@chromium.org changed reviewers:
+ alph@chromium.org
- alph@google.com

[paulirish, 2014-10-07 05:48:39]

On 2014/10/07 at 05:24:25, pdr wrote:
> We're going to run out of space with the invalidations tracking project. What
do you think about making the causes tab show up automatically as default
instead of the details tab?

(Unfortunately my timeline is still busted because of crbug.com/417467 otherwise
I'd love to try out this patch.)

Tall stacks will have more influence on how much vertical space we consume than
the fact that we show stacks (or multiple stacks). 

Looking at http://pr.gg/justcause.png I don't understand what about the
invalidations project necessitates splitting the details view up. 

Details view already does overflow fine: http://goo.gl/9EXZLY
There one bug around the treatment of the pie chart, that we need to address…

But ultimately this tool is far more powerful if we can show all record details
without toggling pane visibility back and forth.

[caseq, 2014-10-07 05:55:26]

On 2014/10/07 05:10:52, paulirish wrote:

> Since details is default tab,

Not really, I've changed that a few weeks ago.

> this means adding another click not just to hunt
> down where this invalidation happened, but adding a click before the user can
> even see where it's from.

It's probably not that bad, since the tab selection is sticky when you jump
between records and, moreover, last tab is the default, so effectively...

> The callstack is probably the most important information in Details view and
> especially now that it's being upgraded, we should have it front and center
for
> the users.

... that's exactly what we get :-) The question rather is, isn't the contents of
the Details tab (which I think we should now rename to Timing or something like
that) gets too cumbersome to access.

[pfeldman, 2014-10-07 06:10:43]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2014-10-07 06:10:43]

@paul: I'd want it to be a single overflowing details sheet, but invalidations
and stacks (consider async ones) tend to take multiple screens in height. So
exploring them turns into a dedicated activity. Hence the tab.

I think we need an overview tab with a little bit of everything and hyperlinks
to more details (pointong to sibling tabs ornother panels) to be thebdefaul one.
It will contain truncated stack for Paul, timing info for Nat, link to
invalidations for P.D.R.

[pfeldman, 2014-10-07 06:11:55]

never.type.on.android.while.walking

[caseq, 2014-10-07 09:50:08]

lgtm with nits

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
File LayoutTests/inspector/tracing/timeline-event-causes.html (right):

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:17: function
setTimeoutFunction(callback) {
{ => next line

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:26: function
finishAndRunNextTest() {
ditto

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:33:
testRequestAnimationFrame();
You probably want to use InspectorTest.runTestSuite() here -- it gets about the
same but keeps the tests unaware of who's next and catches and logs exceptions.

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:39: function
requestAnimationFrameFunction(callback) {
{ => next line

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:40:
requestAnimationFrame(function() {
Drop the "function() { }", just "callback", it's cleaner :)

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:50: function
finishAndRunNextTest() {
{ => next line

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:63: function
styleRecalcFunction(callback) {
ditto

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:74: function
finishAndRunNextTest() {
ditto

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:87: function
layoutFunction(callback) {
ditto

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:99: function
finishAndRunNextTest() {
ditto

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TimelinePanel.js (right):

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1138: if (!event ||
!this._captureCausesSetting.get())
nit: drop !event check

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1499: * @interface
s/@interface/@constructor/

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1505:
this.element.classList.add("timeline-causes-view");
unused?

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1509: setContent:
function(content)
Please annotate @type for content.

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TimelinePanel.js:1518:
this.element.appendChild(content);
This will throw if (!content), so perhaps put under else?

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:695: default:
Nuke default section or move the fall-back strings here?

[pdr., 2014-10-08 06:06:20]

Paul and I chatted today and he made a good case for moving ahead with
everything in this patch except the tab part. This unblocks Yurys, makes
progress towards my invalidations dream, and we can split details into tabs
later if it does become a problem. Also, removing tabs is hard!
(http://crbug.com/100319#c34).

WDYT?

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
File LayoutTests/inspector/tracing/timeline-event-causes.html (right):

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:17: function
setTimeoutFunction(callback) {
On 2014/10/07 at 09:50:07, caseq wrote:
> { => next line

Done (here, and the others).

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:33:
testRequestAnimationFrame();
On 2014/10/07 at 09:50:07, caseq wrote:
> You probably want to use InspectorTest.runTestSuite() here -- it gets about
the same but keeps the tests unaware of who's next and catches and logs
exceptions.

Done.

https://codereview.chromium.org/631573002/diff/40001/LayoutTests/inspector/tr...
LayoutTests/inspector/tracing/timeline-event-causes.html:40:
requestAnimationFrame(function() {
On 2014/10/07 at 09:50:07, caseq wrote:
> Drop the "function() { }", just "callback", it's cleaner :)

Done :)

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/631573002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:695: default:
On 2014/10/07 at 09:50:08, caseq wrote:
> Nuke default section or move the fall-back strings here?

Done.

[paulirish1, 2014-10-08 22:54:32]

Usability-wise I think this is a big win. Nice.

In the future we probably have to invest in some enhanced stack visuals:
+ truncate stack and add [reveal] link
+ coalesce multiple stacks when we have 10 invalidation stacks
+ blackbox treatment 

… but we can address those separately, along with cleaning up the layout details
like Pie chart on singular record.

[pdr., 2014-10-09 17:35:01]

caseq or pfeldman, ping?

[caseq, 2014-10-13 08:43:32]

lgtm once again (sorry about the delay, we both have been OOO).

[pdr., 2014-10-13 17:02:54]

On 2014/10/13 at 08:43:32, caseq wrote:
> lgtm once again (sorry about the delay, we both have been OOO).

No worries, thanks for the reviews!

[pdr., 2014-10-13 17:02:59]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-13 17:03:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631573002/60001

[commit-bot: I haz the power, 2014-10-13 19:28:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-13 19:28:50]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/31544)

[pdr., 2014-10-13 19:40:16]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-13 19:41:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/631573002/60001

[commit-bot: I haz the power, 2014-10-13 21:19:07]

Committed patchset #4 (id:60001) as 183626