Bug 19159 – Inspector should support console.time/console.timeEnd

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

[Adam Roben (:aroben)]

The Inspector should support console.time/console.timeEnd for Firebug parity.

[Mark Rowe (bdash)]

<rdar://problem/5950864>

[Keishi Hattori]

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.

[Keishi Hattori]

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?

[Adam Roben (:aroben)]

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!

[Adam Roben (:aroben)]

(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.

[Keishi Hattori]

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.

[Keishi Hattori]

Created attachment 21978 [details]
Fixed bug.

I changed it to
    void time(in [ConvertUndefinedOrNullToNullString] DOMString title);
and it works now.

[Darin Adler]

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

[Keishi Hattori]

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

[Darin Adler]

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.

[Keishi Hattori]

Created attachment 22006 [details]
reverted

I see. I didn't test it thoroughly enough to notice. I put the FIXME back in.

[Adam Roben (:aroben)]

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!

[Keishi Hattori]

Created attachment 22043 [details]
fixed

fixed

[Adam Roben (:aroben)]

Comment on attachment 22043 [details]
fixed

+    HashMap<String, double> m_times;

I'd still like to use KJS::UString here.

r=me

[Adam Roben (:aroben)]

Comment on attachment 22006 [details]
reverted

Clearing review flag since there's a newer patch available.

[Keishi Hattori]

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?

[Adam Roben (:aroben)]

(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.

[Keishi Hattori]

(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. 

[Mark Rowe (bdash)]

Landed in r35392.