Web Inspector: console.markTimeline vs. console.timeStamp

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Jan Honza Odvarko]

Since console.timeStamp (introduced in Firebug 1.8 beta) does pretty much the same as console.markTimeline in WebKit, I am thinking about adopting the name so, the API is the same across web devtools (certainly better for web-dev folks)
 
The question is whether you would consider renaming console.markTimeline. Or it’s already frozen.
 
The reason is that not all dev tools have to necessarily understand what a “timeline” means. For example, Firebug doesn’t really have an official timeline even if the waterfall graph used on the Net panel is often using that term.

Some tools don’t have to necessarily have something like a timeline at all and they could implement the method for logging purposes only – not displayed graphically, but only for comparing (order) with other logs.


From these reasons the console.timeStamp seems to be more generic and flexible and also more inline with existing API names like:

- time
- timeEnd

What do you think?

I have created a bug report also in the Firebug issue list here:
http://code.google.com/p/fbug/issues/detail?id=4560

Honza

[Pavel Feldman]

I don't mind renaming to timeStamp. I know that many people are using exiting API already, but your concerns seem valid and timeStamp does indeed sound more in line with time and timeStamp. We should probably deprecate markTimeline and log something like "console.markTimeline is deprecated. Please use the console.timeStamp instead." upon its calls for a cycle or two.

[Pavel Feldman]

@Honza do you want to submit a patch :)?

[Jan Honza Odvarko]

(In reply to comment #2)
> @Honza do you want to submit a patch :)?
That's challenging :-)

I believe that this should be very simple change for somebody who is experienced with the code, just search and replace?

I did the search part and I have found following occurrences:

\WebKit\LayoutTests\inspector\timeline\timeline-event-dispatch.html(9):    console.markTimeline("Handling mousedown");
\WebKit\LayoutTests\inspector\timeline\timeline-mark-timeline.html(9):    console.markTimeline("MARK TIMELINE");
\WebKit\LayoutTests\inspector\timeline\resources\timeline-iframe-data.html(4):console.markTimeline("SCRIPT TAG");
\WebKit\Source\WebCore\inspector\front-end\TimelineOverviewPane.js(143):        function markTimeline(record)
\WebKit\Source\WebCore\inspector\front-end\TimelineOverviewPane.js(154):        this._forAllRecords(records, markTimeline.bind(this));
\WebKit\Source\WebCore\page\Console.cpp(256):void Console::markTimeline(PassRefPtr<ScriptArguments> arguments, PassRefPtr<ScriptCallStack>)
\WebKit\Source\WebCore\page\Console.h(72):    void markTimeline(PassRefPtr<ScriptArguments>, PassRefPtr<ScriptCallStack>);
\WebKit\Source\WebCore\page\Console.idl(47):        [CustomArgumentHandling] void markTimeline();

Honza

[Mike West]

I'll take a look at this, Pavel. Seems straightforward enough for a newbie. :)

[Mike West]

Created attachment 100208 [details]
Patch

[WebKit Review Bot]

Comment on attachment 100208 [details]
Patch

Attachment 100208 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9007109

New failing tests:
http/tests/misc/slow-loading-mask.html
http/tests/navigation/javascriptlink-frames.html
fast/blockflow/japanese-rl-selection.html
svg/transforms/text-with-mask-with-svg-transform.svg
fast/backgrounds/background-leakage.html
fast/box-shadow/scaled-box-shadow.html
fast/backgrounds/repeat/negative-offset-repeat.html
svg/W3C-SVG-1.1/struct-use-01-t.svg
transforms/2d/hindi-rotated.html
inspector/timeline/timeline-time-stamp.html
inspector/timeline/timeline-mark-timeline.html
svg/repaint/filter-repaint.svg
fast/blockflow/japanese-lr-selection.html

[WebKit Review Bot]

Created attachment 100209 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mike West]

Created attachment 100215 [details]
I didn't realize we had Chromium platform tests... Now I do.

[Pavel Feldman]

Comment on attachment 100215 [details]
I didn't realize we had Chromium platform tests... Now I do.

View in context: https://bugs.webkit.org/attachment.cgi?id=100215&action=review

One comment, otherwise looks good.

> Source/WebCore/page/Console.cpp:258
> +    String consoleMessage = "console.markTimeline() is deprecated. Please use console.timeStamp() instead.\n";

I can see Closure JS library using this method, I can also see a couple of production web apps using it (Google Maps, GMail, etc.). Extensive console logging will affect site performance, so I'd rather:

- make it silently delegate to timeStamp here
- deprecate it in all the docs possible
- fix Closure and suggest Maps and Gmail migrating to the new solution (they will benefit via Firebug support).

Then we will be able to either put logging here or nuke the method altogether.

[Mike West]

Created attachment 100264 [details]
Dropping console warning.

[Mike West]

I've dropped the logging, and I'll file a bug/patch for Closure shortly.

Can you point me to the documentation you'd like updated? I'm happy to take care of that, if I can, but I'm not entirely sure what you're looking for.

[Pavel Feldman]

(In reply to comment #11)
> I've dropped the logging, and I'll file a bug/patch for Closure shortly.
> 
> Can you point me to the documentation you'd like updated? I'm happy to take care of that, if I can, but I'm not entirely sure what you're looking for.

I'd looks for metions in:
http://code.google.com/chrome/devtools/
http://code.google.com/webtoolkit/speedtracer/logging-api.html

[Pavel Feldman]

Comment on attachment 100264 [details]
Dropping console warning.

View in context: https://bugs.webkit.org/attachment.cgi?id=100264&action=review

> LayoutTests/inspector/timeline/timeline-enum-stability-expected.txt:17
> +    TimeStamp : "TimeStamp"

This is likely to harm SpeedTracer, we should let them know we are breaking api here.

> LayoutTests/inspector/timeline/timeline-time-stamp-expected.txt:1
> +Tests the Timeline API timeStamp feature

It'll need chromium baseline, but WK sheriff will do one for you.

[Mike West]

Comment on attachment 100264 [details]
Dropping console warning.

I've submitted a patch for Closure to try `timeStamp` first and fall back to `markTimeline` for older versions, and the SpeedTracer team has a patch in progress to accept `timeStamp` (http://gwt-code-reviews.appspot.com/1475801/).  I'll follow up with both regarding documentation.

I think this should be safe to land (especially as it won't actually break any code yet), so setting cq?.

[WebKit Review Bot]

Comment on attachment 100264 [details]
Dropping console warning.

Clearing flags on attachment: 100264

Committed r91061: <http://trac.webkit.org/changeset/91061>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.