Bug 19225

Summary: Web Inspector: create automated profiler tests
Product: WebKit Reporter: Kevin McCullough <kmccullough>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Normal CC: graouts, inspector-bugzilla-changes, jonowells, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
timothy: review-
Proposed patch
oliver: review-
Proposed patch none

Description Kevin McCullough 2008-05-23 16:41:29 PDT
In WebCore/manual-tests/inspector there are many manual tests for the new profiler.  It would be great if we could automate them.
Comment 1 Mark Rowe (bdash) 2008-05-24 20:35:11 PDT
*** Bug 19226 has been marked as a duplicate of this bug. ***
Comment 2 Kevin McCullough 2008-10-22 13:05:44 PDT
Created attachment 24566 [details]
Proposed patch
Comment 3 Timothy Hatcher 2008-10-22 13:30:41 PDT
Comment on attachment 24566 [details]
Proposed patch

+    if(iter == end)
+        return jsNull();

I think it would be better to return an empty array instead of null.

+const ProfilesArray& Console::profiles() const
+{
+    Page* page = this->page();
+    ASSERT_ARG(page, page);
+    return page->inspectorController()->profiles();
+}

We need to have the Console object hold the profiles, since there is a Console object per Frame. Asking the InspectorController for the Profiles would return all profiles for all frames, and that is a security issue.

The InspectorController could still have it's own list since it does want all Profiles for all Frames.
Comment 4 Kevin McCullough 2008-10-22 15:25:27 PDT
Created attachment 24574 [details]
Proposed patch
Comment 5 Oliver Hunt 2008-10-22 15:29:34 PDT
Comment on attachment 24574 [details]
Proposed patch

I don't like
 46     if(iter == end)
 47         return constructArray(exec, list);
 48 
 49     do {
 50         list.append(toJS(exec, iter->get()));
 51         ++iter;
 52     } while (iter != end);

i think
ArgList list;
ProfilesArray::const_iterator end = profiles.end();
for (ProfilesArray::const_iterator iter = profiles.begin(); iter != end; ++iter)
    list.append(toJS(exec, iter->get()));

return constructArray(exec, list);

is better as it's simpler, smaller, and more in keeping with the style of similar code elsewhere.
Comment 6 Kevin McCullough 2008-10-22 16:01:00 PDT
Created attachment 24578 [details]
Proposed patch
Comment 7 Kevin McCullough 2008-10-22 16:04:21 PDT
Committed revision 37791.
Comment 8 Kevin McCullough 2008-10-27 12:33:09 PDT
We need to do this better.  Exposing the profiles via JS is not necessarily the best way to go right now.  We don't know the correct format or API of a profile.  A new idea is to let DRT use SPI to get the profiles and we can return to the idea of exposing profiles via JS later.
Comment 9 Timothy Hatcher 2008-10-30 21:46:30 PDT
Comment on attachment 24578 [details]
Proposed patch

Clearing the review flag since this landed. Keeping open based on Kevin's last comment.