Summary: | Web Inspector: console.markTimeline vs. console.timeStamp | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jan Honza Odvarko <odvarko> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
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 | ||||||||||||
Attachments: |
|
Description
Jan Honza Odvarko
2011-06-24 00:55:08 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. @Honza do you want to submit a patch :)? (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 I'll take a look at this, Pavel. Seems straightforward enough for a newbie. :) Created attachment 100208 [details]
Patch
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 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
Created attachment 100215 [details]
I didn't realize we had Chromium platform tests... Now I do.
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. Created attachment 100264 [details]
Dropping console warning.
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. (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 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 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 on attachment 100264 [details] Dropping console warning. Clearing flags on attachment: 100264 Committed r91061: <http://trac.webkit.org/changeset/91061> All reviewed patches have been landed. Closing bug. |