The Inspector should support console.time/console.timeEnd for Firebug parity.
<rdar://problem/5950864>
Created attachment 21971 [details] Proposed patch Maybe needs some error management code but otherwise works. By the way aroben, I found out you were right that Firebug tells you the source file that time or timeEnd was called from. I'll file it.
Created attachment 21973 [details] Slightly better Added error managing code. I tried to match Firebug so that calling time/timeEnd with the title paramenter undefiened gets ignored, but I did title=="undefined" so that doesn't work when you want to pass a string "undefined" as the title. Is there some way to get around this?
Comment on attachment 21973 [details] Slightly better + void time(const String& title); + void timeEnd(const String& title); These functions should take const UString& parameters instead, to avoid an unnecessary conversion from UString -> String. InspectorController::{start,stop}Timing should also take const UString&. +void Console::time(const String& title) +{ + if (title == "undefined") + return; Is this what Firebug does? + double elapsed = page->inspectorController()->stopTiming(title); + if (!isfinite(elapsed)) + return; Will isfinite return false if time() was never called with this title? I think a clearer way of accomplishing this would be something like: bool InspectorController::stopTiming(const UString& title, double& elapsed); And then your code could look like this: double elapsed; if (!page->inspectorController()->stopTiming(title, elapsed)) return; I think you should move time/timeEnd to just below profile/profileEnd in Console.cpp, Console.h, and Console.idl. +void InspectorController::startTiming(const String& title) +{ + m_times.set(title, currentTime() * 1000); +} This will overwrite any existing timer for "title". Is that what Firebug does? + HashMap<String, double> m_times; This should store UStrings instead of Strings to avoid unnecessary string conversions. r- for now so you can address the above, plus: 1. You'll need a ChangeLog entry (it's helpful to write a ChangeLog even for the first version of your patch -- it helps you be clearer in your own mind about what you did, and helps the reviewer understand your patch) 2. I'd like to see some manual tests added to WebCore/manual-tests/inspector. These are useful for making sure that we match Firebug's behavior (or do better than it), and for catching regressions in the future. Looking good!
(In reply to comment #3) > Created an attachment (id=21973) [edit] > I tried to match Firebug so that calling time/timeEnd with the title paramenter > undefiened gets ignored, but I did title=="undefined" so that doesn't work when > you want to pass a string "undefined" as the title. Is there some way to get > around this? I think if you specify your function like this in the IDL: [ConvertUndefinedOrNullToNullString] void time(in DOMString title) then you'll end up with a null string (title.isNull() will return true) when undefined or null is passed to the function. That's probably what you want.
Created attachment 21974 [details] Fixed. I did [ConvertUndefinedOrNullToNullString] but isNull seems to return true. Strange. Below is the test run in Firebug. >>>console.timeEnd("1") >>>console.time("2") >>>console.time("3") >>>console.timeEnd("3") 3: 3ms >>>console.timeEnd("3") >>>console.time() >>>console.timeEnd() >>>console.time("2") >>>console.timeEnd("2") 2: 1014ms 2: If it says ~1000ms the first time is not overwritten.
Created attachment 21978 [details] Fixed bug. I changed it to void time(in [ConvertUndefinedOrNullToNullString] DOMString title); and it works now.
Comment on attachment 21978 [details] Fixed bug. This looks good. For this here, there's a more efficient, but less good looking way to do it: + if (!m_times.contains(title)) + return false; + + double startTime = m_times.take(title); Instead you can do: HashMap<String, double>::iterator it = m_times.find(title); if (it == m_times.end()) return false; double startTime = it->second; m_times.remove(it); The more complicated version does only one hash table lookup and so is more efficient. r=me as-is
Created attachment 21985 [details] fixed mistakes Thank you Darin. I really learn a lot. I realized I've made a few mistakes. I forgot these if (!m_frame) return; and I realized that bug #19791 Inspector should show the source file that called console.time and console.timeEnd was my complete misunderstanding. I think we can show the source files by passing m_frame->loader()->url()->string() as sourceURL
Comment on attachment 21985 [details] fixed mistakes + const KURL& url = m_frame->loader()->url(); This is the URL of the source of the frame. That's not what we want for the console. The console wants the URL and line number of the source code of the call site where we're calling timeEnd from. I don't know the proper mechanism for getting that information from JavaScriptCore.
Created attachment 22006 [details] reverted I see. I didn't test it thoroughly enough to notice. I put the FIXME back in.
Comment on attachment 22006 [details] reverted +void Console::time(const KJS::UString& title) In this .cpp file you shouldn't need the "KJS::" prefix, because near the top of the file is the line: using namespace KJS; which means that prefix is determined automatically by the compiler. + const KURL& url = m_frame->loader()->url(); + + double elapsed; + if (!page->inspectorController()->stopTiming(title, elapsed)) + return; + + String message = String(title) + String::format(": %.0fms", elapsed); + // FIXME: <https://bugs.webkit.org/show_bug.cgi?id=19791> We should pass in the real sourceURL here so that the Inspector can show it. + page->inspectorController()->addMessageToConsole(JSMessageSource, LogMessageLevel, message, 0, url.string()); Looks like we're still passing the page's URL in here. I wonder if it would be better to pass no URL at all, to avoid confusion. +void InspectorController::startTiming(const KJS::UString& title) You don't need "KJS::" in this .cpp file either. + HashMap<String, double> m_times; I still think we should make this store UStrings instead of Strings: HashMap<KJS::UString, double> m_times; Other than that, this looks great! r=me, but feel free to upload a new version that fixes the above issues. Sorry for the delay in reviewing this!
Created attachment 22043 [details] fixed fixed
Comment on attachment 22043 [details] fixed + HashMap<String, double> m_times; I'd still like to use KJS::UString here. r=me
Comment on attachment 22006 [details] reverted Clearing review flag since there's a newer patch available.
When I change that to UString I get the compile errors below /Users/keishi_hattori/WebKit/WebCore/page/InspectorController.h:225: error: invalid use of undefined type 'struct WTF::DefaultHash<KJS::UString>' /Users/keishi_hattori/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/HashFunctions.h:130: error: declaration of 'struct WTF::DefaultHash<KJS::UString>' /Users/keishi_hattori/WebKit/WebCore/page/InspectorController.h:225: error: template argument 3 is invalid is this because a default comparing function for UString is not defined?
(In reply to comment #16) > When I change that to UString I get the compile errors below > > /Users/keishi_hattori/WebKit/WebCore/page/InspectorController.h:225: error: > invalid use of undefined type 'struct WTF::DefaultHash<KJS::UString>' > /Users/keishi_hattori/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/HashFunctions.h:130: > error: declaration of 'struct WTF::DefaultHash<KJS::UString>' > /Users/keishi_hattori/WebKit/WebCore/page/InspectorController.h:225: error: > template argument 3 is invalid > > is this because a default comparing function for UString is not defined? > I think it just means we need to include <kjs/ustring.h> in InspectorController.h.
(In reply to comment #17) > I think it just means we need to include <kjs/ustring.h> in > InspectorController.h. > I did that but I get the same errors. When I read ustring.h, it looks like it only defines a DefaultHash for KJS::UString::Rep*. So I tried that and it gives me fewer errors but I can't get it to compile. I did a grep for HasMap and UString to find some examples but I couldn't find a good one to copy.
Landed in r35392.