| Summary: | Web Inspector: split Console into two entities, a web-facing bound object and page console. | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||||||||||||||||||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Sergey Ryazanov <serya> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, apavlov, buildbot, dglazkov, esprehn+autocc, gyuyoung.kim, haraken, japhet, keishi, loislo, mkwst+watchlist, ojan.autocc, pfeldman, pmuellr, rakuco, rego+ews, rniwa, serya, vsevik, web-inspector-bugs, webkit-ews, webkit.review.bot, xan.lopez, yurys | ||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Pavel Feldman
2013-03-06 07:10:14 PST
Created attachment 191760 [details]
Patch
Comment on attachment 191760 [details] Patch Attachment 191760 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17045262 Comment on attachment 191760 [details] Patch Attachment 191760 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17086053 Comment on attachment 191760 [details] Patch Attachment 191760 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16991311 Comment on attachment 191760 [details] Patch Attachment 191760 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17082223 Comment on attachment 191760 [details] Patch Attachment 191760 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17073145 New failing tests: fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml Created attachment 191786 [details]
Patch
Comment on attachment 191786 [details] Patch Attachment 191786 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16991356 Created attachment 193152 [details]
Patch
Comment on attachment 193152 [details] Patch Attachment 193152 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17189511 Comment on attachment 193152 [details] Patch Attachment 193152 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17129205 Comment on attachment 193152 [details] Patch Attachment 193152 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17041605 Comment on attachment 193152 [details] Patch Attachment 193152 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16996290 New failing tests: fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml Comment on attachment 193152 [details] Patch Attachment 193152 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17126439 Comment on attachment 193152 [details] Patch Attachment 193152 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17186187 Comment on attachment 193152 [details] Patch Attachment 193152 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/16997832 Comment on attachment 193152 [details] Patch Attachment 193152 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17176167 New failing tests: fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml Created attachment 193259 [details]
Patch
Comment on attachment 193259 [details] Patch Attachment 193259 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17215140 Comment on attachment 193259 [details] Patch Attachment 193259 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17116304 Comment on attachment 193259 [details] Patch Attachment 193259 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17210250 Comment on attachment 193259 [details] Patch Attachment 193259 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17055662 Created attachment 193261 [details]
Patch
Comment on attachment 193261 [details] Patch Attachment 193261 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17060887 New failing tests: fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml Created attachment 193275 [details]
Patch
Comment on attachment 193275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193275&action=review > Source/WebCore/bindings/js/JSCustomXPathNSResolver.cpp:82 > + if (console) Nit: I believe the accepted WebKit style here would be to inline the assignment. That is `if (PageConsole* console = m_globalObject->impl()->pageConsole())`. > Source/WebCore/dom/Document.cpp:4819 > + if (p) Nit: Inline the assignment, and prefer complete words for variable names: 'page' rather than 'p'. > Source/WebCore/dom/Document.cpp:4831 > + if (p) Nit: Same as above. > Source/WebCore/page/DOMWindow.h:236 > Console* console() const; Do we still need access to the Console from DOMWindow, or will PageConsole fill the external requirements? It would be nice if external callers only had one option; otherwise they'll likely make the wrong choice. If possible, I'd suggest removing DOMWindow::console, or making it private. Comment on attachment 193275 [details] Patch Attachment 193275 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17060918 Comment on attachment 193275 [details] Patch Attachment 193275 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17145279 New failing tests: fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml Created attachment 193285 [details]
Patch
Comment on attachment 193285 [details] Patch Attachment 193285 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17207241 New failing tests: fast/xsl/xslt-mismatched-tags-in-xslt.xml fast/xsl/xslt-missing-namespace-in-xslt.xml Comment on attachment 193285 [details] Patch Attachment 193285 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17187261 Comment on attachment 193285 [details] Patch Attachment 193285 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17165234 Comment on attachment 193285 [details] Patch Attachment 193285 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17156719 Created attachment 193432 [details]
Patch
Created attachment 193448 [details]
Patch
Comment on attachment 193448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193448&action=review > Source/WebCore/ChangeLog:10 > + No new tests (OOPS!). Please remove this line, otherwise commit queue won't let you commit the patch. > Source/WebCore/bindings/js/JSCustomXPathNSResolver.cpp:-80 > - // FIXME: Pass actual line number and source URL. Please keep FIXME here. > Source/WebCore/dom/Document.cpp:127 > +#include "PageConsole.h" Do we still need to include Console.h here? > Source/WebCore/dom/Document.cpp:4848 > + Page* p = page(); Page* page = ... > Source/WebCore/dom/Document.cpp:4861 > + if (p) Ditto > Source/WebCore/page/Console.h:81 > static void mute(); Should be removed. > Source/WebCore/page/Console.h:82 > static void unmute(); Ditto > Source/WebCore/page/Console.h:-99 > - inline Page* page() const; Why did you remove this? > Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:27 > #include "DOMWindow.h" Could be removed now. > Source/WebCore/xml/XSLTProcessorLibxslt.cpp:30 > #include "DOMWindow.h" Could be removed now. Created attachment 193743 [details]
Patch
Comment on attachment 193743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193743&action=review > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:13641 > \ No newline at end of file I am not sure if newline is expected here. Please revert this change. > Source/WebCore/dom/Document.cpp:4847 > + if (page()) if (Page* page = page()) page->console()->addMessage(source, level, message, requestIdentifier, this); > Source/WebCore/dom/Document.cpp:4858 > + if (page()) Ditto > Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:27 > #include "DOMWindow.h" I still think you don't need to include DOMWindow.h here. > Source/WebCore/xml/XSLTProcessorLibxslt.cpp:30 > #include "DOMWindow.h" Ditto Created attachment 193750 [details]
Patch
Created attachment 193791 [details]
Patch
Comment on attachment 193743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193743&action=review >> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:13641 >> \ No newline at end of file > > I am not sure if newline is expected here. Please revert this change. done >> Source/WebCore/dom/Document.cpp:4847 >> + if (page()) > > if (Page* page = page()) > page->console()->addMessage(source, level, message, requestIdentifier, this); It is not valid C++ code. 'page' on the both sides of the expressions refer to the same variable. It is a pointer and the () operation is not applicable to it. >> Source/WebCore/xml/XSLStyleSheetLibxslt.cpp:27 >> #include "DOMWindow.h" > > I still think you don't need to include DOMWindow.h here. done >> Source/WebCore/xml/XSLTProcessorLibxslt.cpp:30 >> #include "DOMWindow.h" > > Ditto done Created attachment 193804 [details]
Patch
Created attachment 193833 [details]
Patch
Comment on attachment 193833 [details] Patch Clearing flags on attachment: 193833 Committed r146208: <http://trac.webkit.org/changeset/146208> All reviewed patches have been landed. Closing bug. Comment on attachment 193743 [details]
Patch
Clearing obsolete r?
|