[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
https://bugs.webkit.org/show_bug.cgi?id=19159
Attachment 21973: Slightly better
https://bugs.webkit.org/attachment.cgi?id=21973&action=edit
------- 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))
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!
More information about the webkit-reviews
mailing list