Bug 19225 - Web Inspector: create automated profiler tests
Summary: Web Inspector: create automated profiler tests
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 19226 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-05-23 16:41 PDT by Kevin McCullough
Modified: 2016-12-13 15:37 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (7.26 KB, patch)
2008-10-22 13:05 PDT, Kevin McCullough
timothy: review-
Details | Formatted Diff | Diff
Proposed patch (7.73 KB, patch)
2008-10-22 15:25 PDT, Kevin McCullough
oliver: review-
Details | Formatted Diff | Diff
Proposed patch (5.37 KB, patch)
2008-10-22 16:01 PDT, Kevin McCullough
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.