Bug 120695 - Refining the StackIterator callback interface
Summary: Refining the StackIterator callback interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-04 12:59 PDT by Mark Lam
Modified: 2013-09-04 15:38 PDT (History)
4 users (show)

See Also:

Attachments
the patch. (18.28 KB, patch)
2013-09-04 13:16 PDT, Mark Lam 		ggaren: review+
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2013-09-04 12:59:05 PDT 
Addressing Geoff's feedback in https://bugs.webkit.org/show_bug.cgi?id=120564#c17.
Comment 1 Mark Lam 2013-09-04 13:16:59 PDT 
Created attachment 210487 [details]
the patch.

This patch has run the javascriptcore and layout tests with no regressions.
Comment 2 Geoffrey Garen 2013-09-04 13:42:49 PDT 
Comment on attachment 210487 [details]
the patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=210487&action=review

> Source/JavaScriptCore/interpreter/CallFrame.h:292
> +            StackIterator iter(this);
> +            iter.iterate<Functor>(functor);

Why do we still need to instantiate this object here?
Comment 3 EFL EWS Bot 2013-09-04 14:04:42 PDT 
Comment on attachment 210487 [details]
the patch.

Attachment 210487 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1693729
Comment 4 Geoffrey Garen 2013-09-04 14:53:22 PDT 
Comment on attachment 210487 [details]
the patch.

After an in-person conversation:

This patch is an improvement, so I'm going to mark it r+.

Please follow up with a rename from StackIterator to StackVisitor, to avoid confusion with the C++ term "iterator".

I don't think we've achieved the callback style of implementation that we originally discussed, but I'm not sure it's super important to get there at this point.
Comment 5 Mark Lam 2013-09-04 15:33:56 PDT 
(In reply to comment #3)
> (From update of attachment 210487 [details])
> Attachment 210487 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/1693729

The EFL WK2 build failure does not look like it's related to this patch.
Comment 6 Mark Lam 2013-09-04 15:35:11 PDT 
Thanks for the review.  Landed in r155075: <http://trac.webkit.org/r155075>.

Follow up patch to rename StackIterator to StackVisitor to come shortly.
Comment 7 Filip Pizlo 2013-09-04 15:38:48 PDT 
(In reply to comment #4)
> (From update of attachment 210487 [details])
> After an in-person conversation:
> 
> This patch is an improvement, so I'm going to mark it r+.
> 
> Please follow up with a rename from StackIterator to StackVisitor, to avoid confusion with the C++ term "iterator".
> 
> I don't think we've achieved the callback style of implementation that we originally discussed, but I'm not sure it's super important to get there at this point.

I like what we have now.  It achieves the goal of allowing the iterator implementation to change more flexibly without having to make changes to its clients.