[webkit-reviews] review denied: [Bug 19159] Inspector should support console.time/console.timeEnd : [Attachment 21973] Slightly better

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 27 09:33:33 PDT 2008

Adam Roben (aroben) <aroben at apple.com> has denied Keishi Hattori
<casey.hattori at gmail.com>'s request for review:
Bug 19159: Inspector should support console.time/console.timeEnd

Attachment 21973: Slightly better

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

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

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!

More information about the webkit-reviews mailing list