Bug 63317

Summary: Web Inspector: console.markTimeline vs. console.timeStamp
Product: WebKit Reporter: Jan Honza Odvarko <odvarko>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Severity: Normal CC: apavlov, bweinstein, dglazkov, joepeck, keishi, loislo, mkwst, paulirish, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Archive of layout-test-results from ec2-cr-linux-02
I didn't realize we had Chromium platform tests... Now I do.
Dropping console warning. none

Description Jan Honza Odvarko 2011-06-24 00:55:08 PDT
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:

Comment 1 Pavel Feldman 2011-06-24 02:22:45 PDT
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.
Comment 2 Pavel Feldman 2011-06-24 02:24:14 PDT
@Honza do you want to submit a patch :)?
Comment 3 Jan Honza Odvarko 2011-06-24 06:11:55 PDT
(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();

Comment 4 Mike West 2011-07-03 12:24:36 PDT
I'll take a look at this, Pavel. Seems straightforward enough for a newbie. :)
Comment 5 Mike West 2011-07-09 09:16:59 PDT
Created attachment 100208 [details]
Comment 6 WebKit Review Bot 2011-07-09 10:44:21 PDT
Comment on attachment 100208 [details]

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

New failing tests:
Comment 7 WebKit Review Bot 2011-07-09 10:44:27 PDT
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
Comment 8 Mike West 2011-07-09 12:32:36 PDT
Created attachment 100215 [details]
I didn't realize we had Chromium platform tests... Now I do.
Comment 9 Pavel Feldman 2011-07-11 02:31:46 PDT
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.
Comment 10 Mike West 2011-07-11 04:30:08 PDT
Created attachment 100264 [details]
Dropping console warning.
Comment 11 Mike West 2011-07-11 04:36:09 PDT
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.
Comment 12 Pavel Feldman 2011-07-11 04:37:59 PDT
(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:
Comment 13 Pavel Feldman 2011-07-11 04:41:21 PDT
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.
Comment 14 Mike West 2011-07-11 08:05:06 PDT
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?.
Comment 15 WebKit Review Bot 2011-07-15 03:06:46 PDT
Comment on attachment 100264 [details]
Dropping console warning.

Clearing flags on attachment: 100264

Committed r91061: <http://trac.webkit.org/changeset/91061>
Comment 16 WebKit Review Bot 2011-07-15 03:06:55 PDT
All reviewed patches have been landed.  Closing bug.