Bug 111578 – Web Inspector: split Console into two entities, a web-facing bound object and page console.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Otherwise, a lot of logging in WebCore needs to go through the DOMWindow which is unnecessary.

[Pavel Feldman]

Created attachment 191760 [details]
Patch

[Early Warning System Bot]

Comment on attachment 191760 [details]
Patch

Attachment 191760 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17045262

[Build Bot]

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

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 191760 [details]
Patch

Attachment 191760 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17082223

[WebKit Review Bot]

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

[Pavel Feldman]

Created attachment 191786 [details]
Patch

[Build Bot]

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

[Sergey Ryazanov]

Created attachment 193152 [details]
Patch

[Early Warning System Bot]

Comment on attachment 193152 [details]
Patch

Attachment 193152 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17189511

[Early Warning System Bot]

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

[Build Bot]

Comment on attachment 193152 [details]
Patch

Attachment 193152 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17041605

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

Comment on attachment 193152 [details]
Patch

Attachment 193152 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17186187

[EFL EWS Bot]

Comment on attachment 193152 [details]
Patch

Attachment 193152 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/16997832

[WebKit Review Bot]

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

[Sergey Ryazanov]

Created attachment 193259 [details]
Patch

[Early Warning System Bot]

Comment on attachment 193259 [details]
Patch

Attachment 193259 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17215140

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 193259 [details]
Patch

Attachment 193259 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17210250

[Build Bot]

Comment on attachment 193259 [details]
Patch

Attachment 193259 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17055662

[Sergey Ryazanov]

Created attachment 193261 [details]
Patch

[WebKit Review Bot]

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

[Sergey Ryazanov]

Created attachment 193275 [details]
Patch

[Mike West]

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.

[Build Bot]

Comment on attachment 193275 [details]
Patch

Attachment 193275 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17060918

[WebKit Review Bot]

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

[Sergey Ryazanov]

Created attachment 193285 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 193285 [details]
Patch

Attachment 193285 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17187261

[Build Bot]

Comment on attachment 193285 [details]
Patch

Attachment 193285 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17165234

[Build Bot]

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

[Sergey Ryazanov]

Created attachment 193432 [details]
Patch

[Sergey Ryazanov]

Created attachment 193448 [details]
Patch

[Vsevolod Vlasov]

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.

[Sergey Ryazanov]

Created attachment 193743 [details]
Patch

[Vsevolod Vlasov]

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

[Sergey Ryazanov]

Created attachment 193750 [details]
Patch

[Sergey Ryazanov]

Created attachment 193791 [details]
Patch

[Sergey Ryazanov]

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

[Sergey Ryazanov]

Created attachment 193804 [details]
Patch

[Sergey Ryazanov]

Created attachment 193833 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193833 [details]
Patch

Clearing flags on attachment: 193833

Committed r146208: <http://trac.webkit.org/changeset/146208>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vsevolod Vlasov]

Comment on attachment 193743 [details]
Patch

Clearing obsolete r?