Bug 74313 – Implement the JavaScriptCore bindings for eventListenerHandlerLocation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Konrad Piascik]

This method currently has a FIXME and simple returns false.

[Konrad Piascik]

Created attachment 118821 [details]
patch

[Geoffrey Garen]

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  "&&".

[Konrad Piascik]

Created attachment 118822 [details]
amended patch

[Geoffrey Garen]

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.

[Konrad Piascik]

Created attachment 118857 [details]
Use ustringToString to avoid copy

[Geoffrey Garen]

Comment on attachment 118857 [details]
Use ustringToString to avoid copy

r=me

[WebKit Review Bot]

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

[Adam Barth]

Comment on attachment 118857 [details]
Use ustringToString to avoid copy

There's a blank line at the top of your ChangeLog diff.

[Eric Seidel (no email)]

Our ChangeLog parsing could be more robust... but in this case we should just show a nicer error.

[Konrad Piascik]

Created attachment 118911 [details]
Fixed Change Log

[WebKit Review Bot]

Comment on attachment 118911 [details]
Fixed Change Log

Clearing flags on attachment: 118911

Committed r102648: <http://trac.webkit.org/changeset/102648>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch broke Mac builds:
http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Build%29/builds/4318

[Kenneth Russell]

Reverted r102648 for reason:

Broke the Snow Leopard and Windows builds

Committed r102653: <http://trac.webkit.org/changeset/102653>

[Konrad Piascik]

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.

[WebKit Review Bot]

Comment on attachment 118938 [details]
mac build fix

Clearing flags on attachment: 118938

Committed r102656: <http://trac.webkit.org/changeset/102656>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

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>

[Ryosuke Niwa]

Please don't land this patch again until you're sure your patch works on Mac.

[Konrad Piascik]

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.

[Ryosuke Niwa]

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.

[Konrad Piascik]

Created attachment 119068 [details]
Updated ChangeLog

[Konrad Piascik]

Created attachment 119417 [details]
patch

Updated to get Windows and Mac builds working.

[WebKit Review Bot]

Comment on attachment 119417 [details]
patch

Rejecting attachment 119417 [details] from commit-queue.

[e-mail] 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.

[Konrad Piascik]

Comment on attachment 119417 [details]
patch

accidentally set + instead of ?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 119417 [details]
patch

Seems OK, if the JSC folks are OK with exposing ExecutableBase as a symbol.

[WebKit Review Bot]

Comment on attachment 119417 [details]
patch

Clearing flags on attachment: 119417

Committed r103488: <http://trac.webkit.org/changeset/103488>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Adam Roben (:aroben)]

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.