Summary: | Introduce CallWith=Document in binding generator | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||
Component: | WebCore JavaScript | Assignee: | youenn fablet <youennf> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez, cgarcia, commit-queue, esprehn+autocc, kondapallykalyan | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
youenn fablet
2016-03-11 05:26:59 PST
Created attachment 273722 [details]
Patch
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.
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. Thanks for the review. I agree with your comments and will tackle this as a follow-up bug 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 Created attachment 273934 [details]
Patch for landing
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 Committed r198102: <http://trac.webkit.org/changeset/198102> |