Bug 155358 – Introduce CallWith=Document in binding generator

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[youenn fablet]

Some DOM function calls are called with a ScriptExecutionContext& which is later downcasted to a Document&.
It would be an improvement to have the DOM APIs take a Document& directly.

[youenn fablet]

Created attachment 273722 [details]
Patch

[WebKit Commit Bot]

Attachment 273722 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 273722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273722&action=review

Good changes. Can land as is, but many small problems with code clarity that we could fix either before or after doing this.

> Source/WebCore/page/DOMWindow.h:163
> +        void focus(bool allowFocus = false);

I think it would be better to have focus that takes a boolean be a private function, and then have the public focus not take an argument. Should also consider reversing the sense of the boolean, because false does not mean “do not allow focus”. Instead the boolean argument could be “check if focus is allowed”? We need to think about a clear way to express what the boolean means.

> Source/WebCore/page/DOMWindow.h:164
> +        void focus(Document&);

It’s really unclear that this version of focus implements a *less* restrictive rule for when focus is allowed than the one without the document. I think we might want to do something to increase the clarity here, maybe giving the functions different names.

> Source/WebCore/page/DOMWindow.h:167
> +        void close(Document&);

It’s really unclear that this version of close implements a *more* restrictive rule for when close is allowed than the one without the document. Note that this is the opposite situation from focus()! It seems like someone could really easily call the wrong one. We should think about how to name these differently to make the difference clear. Could be as simple as naming the one with the Document& closeFromBindings or closeWithRestrictions or some better name.

> Source/WebCore/page/History.cpp:122
>      ASSERT(isMainThread());

Why assert this after checking m_frame? I suggest moving it to the top.

> Source/WebCore/page/History.h:52
> -    void go(int distance);
> +    void go(int);

I think it’s unclear what the int means without the argument name. Maybe we could come up with a better name than “distance”, but no name at all is unclear.

> Source/WebCore/page/History.h:54
> +    void back(Document&);

Same kind of problem with back, forward, and go as above with focus() and close(). There are versions with and without Document& and it’s unclear how they relate to each other.

> Source/WebCore/page/History.h:56
> +    void go(Document&, int);

I think it’s unclear what the int means without the argument name. Maybe we could come up with a better name than “distance”, but no name at all is unclear.

[youenn fablet]

Thanks for the review.
I agree with your comments and will tackle this as a follow-up bug

[WebKit Commit Bot]

Comment on attachment 273722 [details]
Patch

Rejecting attachment 273722 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 273722, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
est/TestObj.idl
patching file Source/WebCore/page/DOMWindow.cpp
patching file Source/WebCore/page/DOMWindow.h
patching file Source/WebCore/page/DOMWindow.idl
patching file Source/WebCore/page/History.cpp
patching file Source/WebCore/page/History.h
patching file Source/WebCore/page/History.idl
patching file Source/WebCore/testing/Internals.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/975702

[youenn fablet]

Created attachment 273934 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 273934 [details]
Patch for landing

Rejecting attachment 273934 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 273934, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/git.webkit.org/WebKit
   0340364..4f112c0  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 198096 = 034036433660ab15cea2cde650a8315187235392
r198098 = 4f112c0fc72ddc95416b9ad67fcc4f9b5deaeb4a
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/976170

[youenn fablet]

Committed r198102: <http://trac.webkit.org/changeset/198102>