[webkit-reviews] review granted: [Bug 19159] Inspector should support console.time/console.timeEnd : [Attachment 22006] reverted

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Keishi Hattori
<casey.hattori at gmail.com>'s request for review:
Bug 19159: Inspector should support console.time/console.timeEnd
https://bugs.webkit.org/show_bug.cgi?id=19159

Attachment 22006: reverted
https://bugs.webkit.org/attachment.cgi?id=22006&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
+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!