| Summary: | Implement the JavaScriptCore bindings for eventListenerHandlerLocation | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Konrad Piascik <kpiascik> | ||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | barraclough, dbates, eric, ggaren, kbr, oliver, ossy, pfeldman, rniwa, webkit.review.bot, yong.li.webkit, zimmermann | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Konrad Piascik
2011-12-12 11:22:52 PST
Created attachment 118821 [details]
patch
Comment on attachment 118821 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=118821&action=review > Source/WebCore/bindings/js/ScriptEventListener.cpp:117 > + if (!jsFunction && jsFunction->isHostFunction()) I think you meant "||" not "&&". Created attachment 118822 [details]
amended patch
Comment on attachment 118822 [details] amended patch View in context: https://bugs.webkit.org/attachment.cgi?id=118822&action=review > Source/WebCore/bindings/js/ScriptEventListener.cpp:123 > + sourceName = String(funcExecutable->sourceURL().characters()); funcExecutable->sourceURL() is a UString. The preferred way to convert JSC::UString to WTF::String is to call ustringToString(), which will avoid a copy. Created attachment 118857 [details]
Use ustringToString to avoid copy
Comment on attachment 118857 [details]
Use ustringToString to avoid copy
r=me
Comment on attachment 118857 [details] Use ustringToString to avoid copy Rejecting attachment 118857 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: ueue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors self._run(tool, options, state) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run step(tool, options).run(state) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 53, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: http://queues.webkit.org/results/10857004 Comment on attachment 118857 [details]
Use ustringToString to avoid copy
There's a blank line at the top of your ChangeLog diff.
Our ChangeLog parsing could be more robust... but in this case we should just show a nicer error. Created attachment 118911 [details]
Fixed Change Log
Comment on attachment 118911 [details] Fixed Change Log Clearing flags on attachment: 118911 Committed r102648: <http://trac.webkit.org/changeset/102648> All reviewed patches have been landed. Closing bug. This patch broke Mac builds: http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Build%29/builds/4318 Reverted r102648 for reason: Broke the Snow Leopard and Windows builds Committed r102653: <http://trac.webkit.org/changeset/102653> Created attachment 118938 [details]
mac build fix
Updated patch with build fix for mac as well as fixing whitespace in ChangeLog. I also updated the copyright header.
Comment on attachment 118938 [details] mac build fix Clearing flags on attachment: 118938 Committed r102656: <http://trac.webkit.org/changeset/102656> All reviewed patches have been landed. Closing bug. Reverted r102656 for reason: Broke the Mac, Windows and WinCairo builds. We need to look into this patch some more. Committed r102657: <http://trac.webkit.org/changeset/102657> Please don't land this patch again until you're sure your patch works on Mac. Created attachment 119023 [details]
patch
Added 5 new private headers as well as 2 new forward declared headers to get mac compiling with this change.
Comment on attachment 119023 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=119023&action=review > Source/JavaScriptCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). This line should appear before the description but after the bug url. > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). This line should appear before the description but after the bug url. Created attachment 119068 [details]
Updated ChangeLog
Created attachment 119417 [details]
patch
Updated to get Windows and Mac builds working.
Comment on attachment 119417 [details] patch Rejecting attachment 119417 [details] from commit-queue. kpiascik@rim.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. Comment on attachment 119417 [details]
patch
accidentally set + instead of ?
Comment on attachment 118857 [details] Use ustringToString to avoid copy Cleared Geoffrey Garen's review+ from obsolete attachment 118857 [details] so that this bug does not appear in http://webkit.org/pending-commit. Comment on attachment 119417 [details]
patch
Seems OK, if the JSC folks are OK with exposing ExecutableBase as a symbol.
Comment on attachment 119417 [details] patch Clearing flags on attachment: 119417 Committed r103488: <http://trac.webkit.org/changeset/103488> All reviewed patches have been landed. Closing bug. (In reply to comment #28) > (From update of attachment 119417 [details]) > Clearing flags on attachment: 119417 > > Committed r103488: <http://trac.webkit.org/changeset/103488> And a trivial test fix landed in http://trac.webkit.org/changeset/103526. Comment on attachment 119417 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=119417&action=review > Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def:300 > + ?s_info@ExecutableBase@JSC@@2UClassInfo@2@B FYI, this is incorrect. It's been fixed as part of bug 76101. |