[webkit-reviews] review denied: [Bug 61853] Web Inspector: allow opening inspector for existing workers : [Attachment 95585] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jun 1 07:04:20 PDT 2011
Pavel Feldman <pfeldman at chromium.org> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 61853: Web Inspector: allow opening inspector for existing workers
https://bugs.webkit.org/show_bug.cgi?id=61853
Attachment 95585: Patch
https://bugs.webkit.org/attachment.cgi?id=95585&action=review
------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95585&action=review
Overall looks good. r- for no constants, no screenshots. Btw, how do we test
this?
> Source/WebCore/inspector/front-end/WorkerManager.js:77
> + var win = this._workerIdToWindow[workerId];
please do not use abbreviations. workerWindow?
> Source/WebCore/inspector/front-end/WorkerManager.js:85
> + url = url.replace("docked=true&", "");
I thought that "docked" was processed in /chromium/ only. Is it not so?
> Source/WebCore/inspector/front-end/WorkersSidebarPane.js:119
> + workerManager.addEventListener("worker-added", this._workerAdded, this);
These should be constants.
> Source/WebCore/inspector/front-end/WorkersSidebarPane.js:139
> + workerItem.checkbox.checked = false;
I'd rather have a list of links.
More information about the webkit-reviews
mailing list