[Webkit-unassigned] [Bug 107207] Support selectively wrapping DOM accesses from certain V8 contexts.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=107207





--- Comment #9 from Ulfar Erlingsson <ulfar.chromium at gmail.com>  2013-01-22 12:00:02 PST ---
(In reply to comment #8)
> It's hard for me to understand what this patch is doing for two reasons:
> 
> A) All the policy-related code obscures the actual meat of the wrapping and logging mechanism.
> B) All the style and memory errors are very distracting.
> 
> What I'd recommend is to break this change down into a number of smaller steps:
> 
> 1) The first patch would contain the wrapping mechanism, but we would not actually wrap anything.
> 2) The second patch would contain the policy mechanism for controlling the wrapping.
>

Thanks for this feedback.  This is exactly the type of guidance I was hoping for.

> Please also run check-webkit-style before uploading patches.  The reason we have a style guide is to make it easier for people to understand code written by other people.  You'll make it easier for me to understand your code if you follow the style guide.
> 

Yes.  Was developing this in the Chromium tree, so hadn't seen the style problems until just submitting the bug.  Will convert to a tree based on the WebKit pull, and keep things clean.

> It's also likely that we'll need to do a few rounds on each patch to clean up the memory errors.  I can understand if you'd like feedback on the general approach before putting in the detailed work to manage memory correctly, but it's difficult for me to see the larger approach with all these errors.  To give a musical analogy, it's like asking someone what they think of a song playing on the radio when the radio has a lot of static.  In principle, you should be able to appreciate the song separately from the static, but the static is really distracting.
> 

Apologies for the memory management bugs (aka static noise).  I had made a significant effort to make the code leak free (e.g., by using MakeWeak), but I had failed to enable valgrind/heapcheck end-to-end detection.  I must also work on my understanding of the V8/bindings memory management (e.g., why the current prototype->Set(..., v8::FunctionTemplate::New(...)) in batchConfigureCallbacks doesn't leak memory, but assigning to a v8::Handle may).  

> For the first patch, I would encourage you to focus on the core wrapping mechanism, assuming that's the part you're most interested in feedback about.

Yes.  Thanks again for taking the time to look at this.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.