As of now, v8::Object::GetPointerFromInternalField doesn't require handle scope. However, in Debug builds we do some checks before accessing it which require handle scope (as they use GetInternalField method, for example). Thus we need an additional handle scope in debug builds (only).
Created attachment 44551 [details] first take
style-queue ran check-webkit-style on attachment 44551 [details] without any errors.
Comment on attachment 44551 [details] first take Seems like the ASSERTS can be outside of the #ifdef. How much does it cost to create the handle scope unconditionally? I'd rather not have NDEBUG everywhere.... Can we create the handle scope at a lower level where we're already splitting on NDEBUG?
(In reply to comment #3) > (From update of attachment 44551 [details]) > Seems like the ASSERTS can be outside of the #ifdef. Technically yes. However, if I remember it right, one can compile WebKit w/ asserts, but in Release mode, so we'll be in the trouble anyway. Having said that, I'd easily adjust. > How much does it cost to create the handle scope unconditionally? I'd rather not have NDEBUG everywhere.... That's a hot path, and handle scope is only needed due to ASSERTS, so I'd rather not spend a CPU cycle on it. If you don't mind. > Can we create the handle scope at a lower level where we're > already splitting on NDEBUG? Yes, for maybeDOMWrapper, but there is this type check, alas. Overall, I wholeheartedly agree those NDEBUG are ugly. So just let me know how you'd like that to be arranged.
Comment on attachment 44551 [details] first take Ah! I misunderstood. It's the ASSERTS themselves that require the handle scope. Yeah, I don't see a more aesthetic way to do this. Thanks for explaining it to me.
(In reply to comment #5) > (From update of attachment 44551 [details]) > Ah! I misunderstood. It's the ASSERTS themselves that require the handle > scope. Yeah, I don't see a more aesthetic way to do this. Thanks for > explaining it to me. Thanks a lot for review, Adam, cq+'ing.
Comment on attachment 44551 [details] first take Rejecting patch 44551 from commit-queue. Found no modified ChangeLogs, cannot create a commit message. All changes require a ChangeLog. See: http://webkit.org/coding/contributing.html
Comment on attachment 44551 [details] first take Lets see if the failure reproduces. If it does, then I'll file a bug and look at the machine itself.
I'm surprised the failure didn't occur earlier. It looks like svn-apply fails with this patch: bugzilla-tool apply-attachment 44551 [details] I wonder if however you generated this git diff is different than how our scripts expect?
(In reply to comment #10) > I'm surprised the failure didn't occur earlier. It looks like svn-apply fails > with this patch: > > bugzilla-tool apply-attachment 44551 [details] > > I wonder if however you generated this git diff is different than how our > scripts expect? git diff --no-prefix Uploading a new patch (generated with plain git diff). Just in case: zsh/3 127 --% git --version git version 1.5.4.3
Created attachment 44683 [details] plain git diff
style-queue ran check-webkit-style on attachment 44683 [details] without any errors.
Comment on attachment 44683 [details] plain git diff Clearing flags on attachment: 44683 Committed r51995: <http://trac.webkit.org/changeset/51995>
All reviewed patches have been landed. Closing bug.
So yes, git diff --no-prefix was to blame.
I've filed bug 32438 about the svn-apply bug.
Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the "right thing" regarding git patches. See "bugzilla-tool help" for more information. You might find one of those commands useful to you when posting patches in the future.
(In reply to comment #18) > Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the > "right thing" regarding git patches. See "bugzilla-tool help" for more > information. You might find one of those commands useful to you when posting > patches in the future. Thank you very much for pointing it out---never heard of it.