Bug 100973 – [V8] Generalize NodeWrapperMap to be able to handle other sorts of keys

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[V8] Generalize NodeWrapperMap to be able to handle other sorts of keys

[Adam Barth]

Created attachment 171900 [details]
Patch

[Kentaro Hara]

Comment on attachment 171900 [details]
Patch

Looks good.

(We might want to invent a better name than IntrusiveDOMWrapperMap. When at first I looked at the implementation, I couldn't figure out what Intrusive implies.)

[Adam Barth]

Actually, I plan to eventually remove this class to avoid the virtual function call, but that's a bit further down this path.

[WebKit Review Bot]

Comment on attachment 171900 [details]
Patch

Rejecting attachment 171900 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ts/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Unreviewed, marking tests as failing preemptively.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 154.

Full output: http://queues.webkit.org/results/14691058

[WebKit Review Bot]

Comment on attachment 171900 [details]
Patch

Clearing flags on attachment: 171900

Committed r133244: <http://trac.webkit.org/changeset/133244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Stephen White]

Reverted r133244 for reason:

Broke Chromium Mac (clang) builds.

Committed r133256: <http://trac.webkit.org/changeset/133256>

[Stephen White]

Sorry if this revert was a misfire -- there was a warning-treated-as-error in clang that I couldn't figure out how to suppress (or if it was a true bug).

[noel gordon]

The specific clang warning-treated-as-error for reference

... d-bad-constructs -Xclang -plugin-arg-find-bad-constructs -Xclang skip-virtuals-in-implementations -fcolor-diagnostics -fno-strict-aliasing -include obj/third_party/WebKit/Source/WebCore/webcore_remaining.WebCorePrefix.h-cc -c ../../third_party/WebKit/Source/WebCore/bindings/v8/V8GCController.cpp -o obj/third_party/webkit/source/webcore/bindings/v8/webcore_remaining.v8gccontroller.o
In file included from ../../third_party/WebKit/Source/WebCore/bindings/v8/V8GCController.cpp:40:
../../third_party/WebKit/Source/WebCore/bindings/v8/IntrusiveDOMWrapperMap.h:60:25: error: unused variable 'info' [-Werror,-Wunused-variable]
        MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::Binding);
                        ^
1 error generated.

[Adam Barth]

Created attachment 172090 [details]
Patch for landing

[Adam Barth]

> The specific clang warning-treated-as-error for reference

Thanks.  This patch didn't actually touch that line of code, but I can fix it anyway.  :)

[WebKit Review Bot]

Comment on attachment 172090 [details]
Patch for landing

Rejecting attachment 172090 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14712086

[Stephen White]

r=me.  Thanks for the fix (don't know why this line didn't fail earlier -- maybe a new version of clang since this file was last compiled?)

[Tony Chang]

(In reply to comment #13)
> r=me.  Thanks for the fix (don't know why this line didn't fail earlier -- maybe a new version of clang since this file was last compiled?)

The cr-linux bots use gcc, so you won't see clang warnings on the ews cr-linux or cq bots.

[Stephen White]

(In reply to comment #14)
> (In reply to comment #13)
> > r=me.  Thanks for the fix (don't know why this line didn't fail earlier -- maybe a new version of clang since this file was last compiled?)
> 
> The cr-linux bots use gcc, so you won't see clang warnings on the ews cr-linux or cq bots.

Right, what I meant was why Adam's change triggered a clang compile failure, since that line existed before.

[WebKit Review Bot]

Comment on attachment 172090 [details]
Patch for landing

Clearing flags on attachment: 172090

Committed r133331: <http://trac.webkit.org/changeset/133331>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.