Bug 90675 – Web Inspector: implement testing harness for pure protocol tests.

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

[Pavel Feldman]

As of today, all inspector tests are based on the existing front-end code. They are loaded into the inspector context and are executed there. Now that WebKit commits to supporting the WebKit Remote Debugging Protocol (that is used both for remote and mobile debugging as well as by third party front-ends), we need a reliable way of testing it.

The way I see it implemented is following:
1) There is a new LayoutTests/inspector/protocol folder containing new tests
2) These tests are working the same way as the regular LayoutTests/inspector ones, except for they are using a different front-end page (not inspector.html, but protocol-test.html).
3) We seed protocol SDK there (InspectorBackend.js and InspectorBackendCommands.js are listed in protocol-test.html)
4) Instead of inspector-test.js, we use a stripped version of it called protocol-test.js
5) Upon runTest, similarly to inspector-test, test code is injected into the front-end (protocol-test.html in our case) where it executes without the real inspector front-end.

[Yury Semikhatsky]

(In reply to comment #0)
> 1) There is a new LayoutTests/inspector/protocol folder containing new tests
I think we will need LayoutTests/inspector-protocol as we will use a alternative front-end for the protocol tests.

[Vivek Galatage]

If it's ok with you Pavel and Yury, I would like to work on this.

[Pavel Feldman]

> I think we will need LayoutTests/inspector-protocol as we will use a alternative front-end for the protocol tests.

+1

The downside of the whole proposal is that we need to start opening alternate front-end page for all platforms. This involves patching DRTs and LayoutTestControllers everywhere. And it might come at high cost.

Alternatively, we could implement window.internals method that would create a Page, open protocol-test.html there and attach to the InspectorController (you would need to implement your own ancestor of InspectorClientLocal). This would be a slicker solution, but we need to learn if we can programmatically open a page (with or without web view) like that. If it works, I would vote for this solution.

[Pavel Feldman]

(In reply to comment #2)
> If it's ok with you Pavel and Yury, I would like to work on this.

Absolutely!

[Vivek Galatage]

I am looking for existing test harness for the existing front-end. In this regard any pointers/directions would be really of great help. Also are there any already existing protocol tests that I can refer to?

[Ilya Tikhonovsky]

I'd start from extending DRT for such new kind of inspector tests.
The current tests for the inspector uses this methods.

http://code.google.com/p/chromium/source/search?q=%5C%2Finspector%5C%2F+file%3ADumpRenderTree&origq=%5C%2Finspector%5C%2F+file%3ADumpRenderTree&btnG=Search+Trunk

When the inspector is opened the test injects the testing code into inspector with help of layoutTestController.evaluateInWebInspector

http://code.google.com/p/chromium/source/search?q=evaluateInWebInspector+&origq=evaluateInWebInspector+&btnG=Search+Trunk

[Vivek Galatage]

(In reply to comment #6)
> I'd start from extending DRT for such new kind of inspector tests.
> The current tests for the inspector uses this methods.
> 
> http://code.google.com/p/chromium/source/search?q=%5C%2Finspector%5C%2F+file%3ADumpRenderTree&origq=%5C%2Finspector%5C%2F+file%3ADumpRenderTree&btnG=Search+Trunk
> 
> When the inspector is opened the test injects the testing code into inspector with help of layoutTestController.evaluateInWebInspector
> 
> http://code.google.com/p/chromium/source/search?q=evaluateInWebInspector+&origq=evaluateInWebInspector+&btnG=Search+Trunk

Thank you for the references.

Based on these pointers, I would like to know how and where the "devtools.html" is loaded in the inspector page's main frame for chromium.

I checked the implementation for WebDevToolsAgentImpl::openInspectorFrontend()in WebKit/chromium/src/WebDevToolsAgentImpl.cpp but this method is empty. 

For Safari(windows port), its present in WebInspectorClient::openInspectorFrontend() and this tries to open inspector.html.

[Yury Semikhatsky]

(In reply to comment #7)
> Based on these pointers, I would like to know how and where the "devtools.html" is loaded in the inspector page's main frame for chromium.
>
> I checked the implementation for WebDevToolsAgentImpl::openInspectorFrontend()in WebKit/chromium/src/WebDevToolsAgentImpl.cpp but this method is empty. 
> 
There are two different codepaths for loading devtools.html:
1) For the layout tests see http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Tools/DumpRenderTree/chromium/TestShell.cpp&exact_package=chromium&q=showDevTools%5C(&type=cs&l=196
2) For the regular DevTools window it is http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/debugger/devtools_window.cc&exact_package=chromium&q=DevToolsWindow&type=cs&l=170

[Pavel Feldman]

Capturing the IRC discussion here:

So the (ideal) plan would be:

1) Implement window.internals.openInspectorTestFrontend

This method will create a Page, install the InspectorFrontendClientLocal-based harness on it (a-la Qt or GTK port). This should require almost no code.

2) Complete the refactoring stated in https://bugs.webkit.org/show_bug.cgi?id=91196. After that, we'll be able to call connectFrontend with the transport from our new test harness.

3) Figure out a way to evaluate InspectorBackend.js and InspectorBackendCommands.js in the context of the new front-end Page. We can use .js -> .h method that is currently used for InjectedScriptSource.js. It creates a header from the .js file with the string constant that can be used from native code.

4) Protocol layout tests will include the modified / stripped version of the inspector-test.js. This version will use new

window.internal.openDummyInspectorFrontend() instead of testRunner.showWebInspector() and same for closeWebInspector.

5) testRunner.evaluateInWebInspector should continue functioning.

6) New tests will live under LayoutTests/inspector-enabled. That's the folder that allows inspector tests, but does not open inspector automatically. DRTs should be modified to do window.internal.closeDummyInspectorFrontend after tests from that folder. 

7) After that, some of the tests (that only talk to the backend), could be moved from LayoutTests/inspector to LayoutTests/inspector-enabled and start using new harness.

[Vivek Galatage]

(In reply to comment #9)
> Capturing the IRC discussion here:
> 
> So the (ideal) plan would be:
> 
> 1) Implement window.internals.openInspectorTestFrontend
> 
> This method will create a Page, install the InspectorFrontendClientLocal-based harness on it (a-la Qt or GTK port). This should require almost no code.
> 

Will this be a raw page from Page.h? If yes, then should we pass all the clients via PageClients as null(0) to this new page?

I have the following pseudo code in my mind about this:

Internals.cpp
=============
class InspectorFrontendClientDummy : public InspectorFrontendClientLocal
{
public:
    // Most of the methods go notImplemented() here
};

class InspectorFrontendChannelDummy : public InspectorFrontendChannel
{
public:
    virtual bool sendMessageToFrontend(const String& message) {
        // We can eval script here.
        // WebInspector.dispatchMessageFromBackend(message)
    }
};

boolean Internals::openDummyInspectorFrontend() {
    Page::PageClients pageClients;
    pageClients.chromeClient = 0;
    pageClients.contextMenuClient = 0;
    pageClients.editorClient = 0;
    pageClients.dragClient = 0;
    pageClients.inspectorClient = 0;
    m_page = new Page(pageClients); 

    m_frontendClient = new InspectorFrontendClientDummy(...); 

    m_page->inspectorController()->setInspectorFrontendClient(m_frontendClient);

    m_frontendChannel = new InspectorFrontendChannelDummy(...);

    m_page->inspectorController()->connectFrontend(m_frontendChannel);
}


Now my questions are:
1. Can the internals object hold these members: m_frontendClient, m_page, m_frontendChannel?
2. If yes, when should these be destroyed?(in Internals object desctrutor?)
3. Other possibility is we can make the m_frontendChannel member of InspectorFrontendClientDummy and destroy it as soon as closeWindow() is called.
4. How to handle the remote frontend? (or do we need to handle the remote case as I am not aware whether these tests can be run on remote devices?)

Please let me know your thoughts on the same. Thank you.

[Pavel Feldman]

(In reply to comment #10)
> (In reply to comment #9)
> > Capturing the IRC discussion here:
> > 
> > So the (ideal) plan would be:
> > 
> > 1) Implement window.internals.openInspectorTestFrontend
> > 
> > This method will create a Page, install the InspectorFrontendClientLocal-based harness on it (a-la Qt or GTK port). This should require almost no code.
> > 
> 
> Will this be a raw page from Page.h? If yes, then should we pass all the clients via PageClients as null(0) to this new page?
> 

Yes, ideally, we should be able to instantiate a page. But passing 0 clients will not work. Sometimes you will get away with 0, sometimes you need to instantiate the ones from EmptyClients, but in some cases you might need stub implementation. Instantiating a page was a major risk in this approach, but I think we should take it.

> I have the following pseudo code in my mind about this:
> 
> Internals.cpp
> =============
> class InspectorFrontendClientDummy : public InspectorFrontendClientLocal
> {
> public:
>     // Most of the methods go notImplemented() here
> };
> 
> class InspectorFrontendChannelDummy : public InspectorFrontendChannel
> {
> public:
>     virtual bool sendMessageToFrontend(const String& message) {
>         // We can eval script here.
>         // WebInspector.dispatchMessageFromBackend(message)
>     }
> };
> 
> boolean Internals::openDummyInspectorFrontend() {
>     Page::PageClients pageClients;
>     pageClients.chromeClient = 0;
>     pageClients.contextMenuClient = 0;
>     pageClients.editorClient = 0;
>     pageClients.dragClient = 0;
>     pageClients.inspectorClient = 0;
>     m_page = new Page(pageClients); 
> 
>     m_frontendClient = new InspectorFrontendClientDummy(...); 
> 
>     m_page->inspectorController()->setInspectorFrontendClient(m_frontendClient);
> 
>     m_frontendChannel = new InspectorFrontendChannelDummy(...);

You should not need to do this as long as you instantiate InspectorFrontendClientLocal with the right Page* frontendPage instance that you create. Then you just pass its instance into the connectFrontend.

> 
>     m_page->inspectorController()->connectFrontend(m_frontendChannel);
> }
> 
> 
> Now my questions are:
> 1. Can the internals object hold these members: m_frontendClient, m_page, m_frontendChannel?

I don't see why not as long as it maintains proper ownership / lifetime for the objects.

> 2. If yes, when should these be destroyed?(in Internals object desctrutor?)

You should create everything upon window.internals.showBlankInspectorFrontend and destroy everything upon window.internals.closeBlankInspectorFrontend.

> 3. Other possibility is we can make the m_frontendChannel member of InspectorFrontendClientDummy and destroy it as soon as closeWindow() is called.

I am not sure you will need this dummy guy.

> 4. How to handle the remote frontend? (or do we need to handle the remote case as I am not aware whether these tests can be run on remote devices?)
> 

There is no such thing, you should not worry about it here.

> Please let me know your thoughts on the same. Thank you.

This all sounds reasonable!

[Vivek Galatage]

Thank you Pavel for your feedback. One question I have as below - 

> > boolean Internals::openDummyInspectorFrontend() {
> >     Page::PageClients pageClients;
> >     pageClients.chromeClient = 0;
> >     pageClients.contextMenuClient = 0;
> >     pageClients.editorClient = 0;
> >     pageClients.dragClient = 0;
> >     pageClients.inspectorClient = 0;
> >     m_page = new Page(pageClients); 
> > 
> >     m_frontendClient = new InspectorFrontendClientDummy(...); 
> > 
> >     m_page->inspectorController()->setInspectorFrontendClient(m_frontendClient);
> > 
> >     m_frontendChannel = new InspectorFrontendChannelDummy(...);
> 
> You should not need to do this as long as you instantiate InspectorFrontendClientLocal with the right Page* frontendPage instance that you create. Then you just pass its instance into the connectFrontend.

So are you hinting at InspectorFrontendClientDummy inheriting both InspectorFrontendClientLocal as well as InspectorFrontendChannel and keeping a raw pointer to the Page* for the frontend?

[Pavel Feldman]

> So are you hinting at InspectorFrontendClientDummy inheriting both InspectorFrontendClientLocal as well as InspectorFrontendChannel and keeping a raw pointer to the Page* for the frontend?

I was, but now that I've looked at the InspectorFrontendClientLocal, I realized that it does not have enough logic. So you should either proceed as you planned.

Your sendMessageToFrontend should call InspectorClient::doDispatchMessageOnFrontendPage.

It is by nature static utility method exposed on the client for reuse only. It can be declared static.

[Vivek Galatage]

Created attachment 154316 [details]
Work-in-progress patch

[WebKit Review Bot]

Comment on attachment 154316 [details]
Work-in-progress patch

Attachment 154316 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13345189

[Gyuyoung Kim]

Comment on attachment 154316 [details]
Work-in-progress patch

Attachment 154316 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13348205

[Yury Semikhatsky]

Comment on attachment 154316 [details]
Work-in-progress patch

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

> Source/WebCore/testing/Internals.cpp:132
> +    InspectorFrontendChannelDummy(Page*);

Mising explicit.

> Source/WebCore/testing/Internals.cpp:1073
> +    pc.chromeClient = new EmptyChromeClient();

All these clients must be deleted when the page closes.

> Source/WebCore/testing/Internals.cpp:1091
> +    delete m_frontendPage;

Please use OwnPtr instead of a raw pointer for storing the page to avoid possible memory leaks if closeDummyInspectorFrontend is not called. Also we should probably enforce the frontend closing when test completes.

[Hajime Morrita]

Comment on attachment 154316 [details]
Work-in-progress patch

From the Internals viewpoint:

- It would be reasonable to have InspectorFrontendChannelDummy to be inside WebCore, not WebCoreTesting
   because it requires many symbols to be exported, which is hard to maintain.
   It could be called TestInspectorFrontendChannel IMHO.
- Same can be said to open/closeDummyInspectorFrontend() implementation.
   (It's totally fine to have the API on Internals though. I'm talking about the code)
- If you are going to add some test specific logic to InspectorFrontendChannelDummy, 
   It could be extracted as a "Client" interface (in WebCore) and its implementation (inside (WebCoreTestSupport)
   Inheriting WebCore class outside WebCore isn't good idea due to its tight coupling.
   Typical pattern in WebKit land is to define pure virtual class called "Client" inside WebCore
   and implement it outside.

We keep test specific code outside WeCore due to two reasons:
- It makes WebCore small
- It prevents accidental security problem.
This change is small enough and doesn't contains risky bits.
So I think it's OK to keep many of them in WebCore unless there is any other intention to keep it outside.

For the same reason, I think it's OK to build InspectorFrontendClientLocal.cpp into WebCore.

[Yury Semikhatsky]

(In reply to comment #18)
> (From update of attachment 154316 [details])
> From the Internals viewpoint:
> 
> - It would be reasonable to have InspectorFrontendChannelDummy to be inside WebCore, not WebCoreTesting
>    because it requires many symbols to be exported, which is hard to maintain.
>    It could be called TestInspectorFrontendChannel IMHO.
> - Same can be said to open/closeDummyInspectorFrontend() implementation.
>    (It's totally fine to have the API on Internals though. I'm talking about the code)
> - If you are going to add some test specific logic to InspectorFrontendChannelDummy, 
>    It could be extracted as a "Client" interface (in WebCore) and its implementation (inside (WebCoreTestSupport)
>    Inheriting WebCore class outside WebCore isn't good idea due to its tight coupling.
>    Typical pattern in WebKit land is to define pure virtual class called "Client" inside WebCore
>    and implement it outside.
> 
> We keep test specific code outside WeCore due to two reasons:
> - It makes WebCore small
> - It prevents accidental security problem.
> This change is small enough and doesn't contains risky bits.
> So I think it's OK to keep many of them in WebCore unless there is any other intention to keep it outside.
> 
> For the same reason, I think it's OK to build InspectorFrontendClientLocal.cpp into WebCore.

As I understand window.internals is supposed to be tightly coupled with WebCore and the problem here seems to be that we link it separately from the rest of WebCore and thus need to export WebCore symbols used in Internals.cpp. Now to overcome this you suggest we should move TestInspectorFrontendChannel and open/closeDummyInspectorFrontend into the WebCore. This seems wrong to me as their implementation is only used in tests. Can we instead have linker consider WebCore/testing as part of the WebCore library in case of layout tests?

[Yury Semikhatsky]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 154316 [details] [details])
> > From the Internals viewpoint:
> > 
> > - It would be reasonable to have InspectorFrontendChannelDummy to be inside WebCore, not WebCoreTesting
> >    because it requires many symbols to be exported, which is hard to maintain.
> >    It could be called TestInspectorFrontendChannel IMHO.
> > - Same can be said to open/closeDummyInspectorFrontend() implementation.
> >    (It's totally fine to have the API on Internals though. I'm talking about the code)
> > - If you are going to add some test specific logic to InspectorFrontendChannelDummy, 
> >    It could be extracted as a "Client" interface (in WebCore) and its implementation (inside (WebCoreTestSupport)
> >    Inheriting WebCore class outside WebCore isn't good idea due to its tight coupling.
> >    Typical pattern in WebKit land is to define pure virtual class called "Client" inside WebCore
> >    and implement it outside.
> > 
> > We keep test specific code outside WeCore due to two reasons:
> > - It makes WebCore small
> > - It prevents accidental security problem.
> > This change is small enough and doesn't contains risky bits.
> > So I think it's OK to keep many of them in WebCore unless there is any other intention to keep it outside.
> > 
> > For the same reason, I think it's OK to build InspectorFrontendClientLocal.cpp into WebCore.
> 
> As I understand window.internals is supposed to be tightly coupled with WebCore and the problem here seems to be that we link it separately from the rest of WebCore and thus need to export WebCore symbols used in Internals.cpp. Now to overcome this you suggest we should move TestInspectorFrontendChannel and open/closeDummyInspectorFrontend into the WebCore. This seems wrong to me as their implementation is only used in tests. Can we instead have linker consider WebCore/testing as part of the WebCore library in case of layout tests?

Otherwise we may end up with a WebCore internal API for WebCore/testing and have test support scattered through the WebCore codebase.

[Vivek Galatage]

> For the same reason, I think it's OK to build InspectorFrontendClientLocal.cpp into WebCore.

Are you referring to chromium build system?

[Vivek Galatage]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 154316 [details] [details])
> > From the Internals viewpoint:
> > 
> > - It would be reasonable to have InspectorFrontendChannelDummy to be inside WebCore, not WebCoreTesting
> >    because it requires many symbols to be exported, which is hard to maintain.
> >    It could be called TestInspectorFrontendChannel IMHO.
> > - Same can be said to open/closeDummyInspectorFrontend() implementation.
> >    (It's totally fine to have the API on Internals though. I'm talking about the code)
> > - If you are going to add some test specific logic to InspectorFrontendChannelDummy, 
> >    It could be extracted as a "Client" interface (in WebCore) and its implementation (inside (WebCoreTestSupport)
> >    Inheriting WebCore class outside WebCore isn't good idea due to its tight coupling.
> >    Typical pattern in WebKit land is to define pure virtual class called "Client" inside WebCore
> >    and implement it outside.
> > 
> > We keep test specific code outside WeCore due to two reasons:
> > - It makes WebCore small
> > - It prevents accidental security problem.
> > This change is small enough and doesn't contains risky bits.
> > So I think it's OK to keep many of them in WebCore unless there is any other intention to keep it outside.
> > 
> > For the same reason, I think it's OK to build InspectorFrontendClientLocal.cpp into WebCore.
> 
> As I understand window.internals is supposed to be tightly coupled with WebCore and the problem here seems to be that we link it separately from the rest of WebCore and thus need to export WebCore symbols used in Internals.cpp. Now to overcome this you suggest we should move TestInspectorFrontendChannel and open/closeDummyInspectorFrontend into the WebCore. This seems wrong to me as their implementation is only used in tests. Can we instead have linker consider WebCore/testing as part of the WebCore library in case of layout tests?

The problem is during the linking phase. Even if we made the WebCoreTestSupport part of the WebCore, it will still be a static library. And the library compiles these cpp files and keeps the .o files in the lib. But actual linking happens during the building of DumpRenderTree binary. Hence the symbols are needed here. DRT links with the WebCoreTestSupport and WebKit2.dll. So essentially the WebCore is available for linking but just because the symbols is missing from the def file, its thrown as a linker error. 

I tried the other option of linking WebCore separately with DRT but again only this library is not enough as now I get some 500+ linker errors. So I guess it expects much more libraries to be linked.

[Hajime Morrita]

(In reply to comment #19)
> As I understand window.internals is supposed to be tightly coupled with WebCore and the problem here seems to be that we link it separately from the rest of WebCore and thus need to export WebCore symbols used in Internals.cpp. Now to overcome this you suggest we should move TestInspectorFrontendChannel and open/closeDummyInspectorFrontend into the WebCore. This seems wrong to me as their implementation is only used in tests. Can we instead have linker consider WebCore/testing as part of the WebCore library in case of layout tests?

Makes sense. My point here is that it's worth pursing to minimize the number of exported symbols.
I agree that that doesn't justify the layering violation.

How about this?
- Setting up PageClients with empty client objects looks common enough to make it a 
   method of PageClients. I guess some other empty client usecases could share it later. 
- InspectorFrontendClientDummy could be implemented as a client object of InspectorFrontendClienLocal,
   rather than a subclass of it.
   Unless amount o exported symbols from InspectorFrontendClienLocal is massive.
   Same can be said for InspectorFrontendChannelLocal.

What do you think?

[Yury Semikhatsky]

(In reply to comment #23)
> (In reply to comment #19)
> > As I understand window.internals is supposed to be tightly coupled with WebCore and the problem here seems to be that we link it separately from the rest of WebCore and thus need to export WebCore symbols used in Internals.cpp. Now to overcome this you suggest we should move TestInspectorFrontendChannel and open/closeDummyInspectorFrontend into the WebCore. This seems wrong to me as their implementation is only used in tests. Can we instead have linker consider WebCore/testing as part of the WebCore library in case of layout tests?
> 
> Makes sense. My point here is that it's worth pursing to minimize the number of exported symbols.
> I agree that that doesn't justify the layering violation.
> 
> How about this?
> - Setting up PageClients with empty client objects looks common enough to make it a 
>    method of PageClients. I guess some other empty client usecases could share it later. 
Given that PageClients' creator should keep references to the clients and delete them when the Page is destroyed I don't think it is a good idea to have a method that would create some set of empty clients that later should be adopted by the caller. This would be error-prone as you could easily create one more empty client and forget to delete it in one of the callers. Are there many cases when PageClients needs to be initialized with empty clients?

> - InspectorFrontendClientDummy could be implemented as a client object of InspectorFrontendClienLocal,
>    rather than a subclass of it.
>    Unless amount o exported symbols from InspectorFrontendClienLocal is massive.
InspectorFrontendClienLocal is a common base implementation for all in-process InspectorFrontendClients and it seems logical to subclass from it instead of introducing additional delegate interface. The only problem it would solve is reducing number of exported symbols used by the Internals object. And if we expect Internals to be tightly coupled with the WebCore code I don't think we should introduce additional abstractions just to reduce the number of symbols exported from WebCore to Internals.

>    Same can be said for InspectorFrontendChannelLocal.
> 
There is no such class.

[Hajime Morrita]

share it later. 
> Given that PageClients' creator should keep references to the clients and delete them when the Page is destroyed I don't think it is a good idea to have a method that would create some set of empty clients that later should be adopted by the caller. This would be error-prone as you could easily create one more empty client and forget to delete it in one of the callers. Are there many cases when PageClients needs to be initialized with empty clients?
> 
There is a few. And I just found we already have such one: 
fillWithEmptyClients() in EmptyClients.h. Is this useful in this case?


> > - InspectorFrontendClientDummy could be implemented as a client object of 
InspectorFrontendClienLocal,
> >    rather than a subclass of it.
> >    Unless amount o exported symbols from InspectorFrontendClienLocal is massive.
> InspectorFrontendClienLocal is a common base implementation for all in-process InspectorFrontendClients and it seems logical to subclass from it instead of introducing additional delegate interface. The only problem it would solve is reducing number of exported symbols used by the Internals object. And if we expect Internals to be tightly coupled with the WebCore code I don't think we should introduce additional abstractions just to reduce the number of symbols exported from WebCore to Internals.
> 
Makes sense. I wasn't aware of such a big picture.
Thanks for your clarification!

> >    Same can be said for InspectorFrontendChannelLocal.
> > 
> There is no such class.
Oops. I'm sorry. I was confused.

[Yury Semikhatsky]

(In reply to comment #25)
> share it later. 
> > Given that PageClients' creator should keep references to the clients and delete them when the Page is destroyed I don't think it is a good idea to have a method that would create some set of empty clients that later should be adopted by the caller. This would be error-prone as you could easily create one more empty client and forget to delete it in one of the callers. Are there many cases when PageClients needs to be initialized with empty clients?
> > 
> There is a few. And I just found we already have such one: 
> fillWithEmptyClients() in EmptyClients.h. Is this useful in this case?
> 
The function doesn't create EmptyChromeClient, apart from that it seems reasonable to set empty clients with it. It deliberately leaks some clients so we won't need to take care of deleting them.

[Hajime Morrita]

(In reply to comment #26)
> (In reply to comment #25)
> > share it later. 
> > > Given that PageClients' creator should keep references to the clients and delete them when the Page is destroyed I don't think it is a good idea to have a method that would create some set of empty clients that later should be adopted by the caller. This would be error-prone as you could easily create one more empty client and forget to delete it in one of the callers. Are there many cases when PageClients needs to be initialized with empty clients?
> > > 
> > There is a few. And I just found we already have such one: 
> > fillWithEmptyClients() in EmptyClients.h. Is this useful in this case?
> > 
> The function doesn't create EmptyChromeClient, apart from that it seems reasonable to set empty clients with it. It deliberately leaks some clients so we won't need to take care of deleting them.

So it won't fit your use case.
I have no strong recommendation to use it.

[Vivek Galatage]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > share it later. 
> > > > Given that PageClients' creator should keep references to the clients and delete them when the Page is destroyed I don't think it is a good idea to have a method that would create some set of empty clients that later should be adopted by the caller. This would be error-prone as you could easily create one more empty client and forget to delete it in one of the callers. Are there many cases when PageClients needs to be initialized with empty clients?
> > > > 
> > > There is a few. And I just found we already have such one: 
> > > fillWithEmptyClients() in EmptyClients.h. Is this useful in this case?
> > > 
> > The function doesn't create EmptyChromeClient, apart from that it seems reasonable to set empty clients with it. It deliberately leaks some clients so we won't need to take care of deleting them.
> 
> So it won't fit your use case.
> I have no strong recommendation to use it.

I have added the creation of EmptyChromeClient with https://bugs.webkit.org/show_bug.cgi?id=92703

This will minimize the number of export symbols required as we can only export fillWithEmptyClients method instead of other exports.

[Vivek Galatage]

As now we need to load the inspector-protocol.html in the newly created frontend page, I am thinking of following design approach. I would be glad to receive the feedback on the same.

I am planning to introduce a new client corresponding to Resource management. I am calling it a ResourceClient. 

ResourceClient.h
================
class ResourceClient {    
public:
    typdef enum {
        INSPECTOR_URL,
        INSPECTOR_PROTOCOL_URL
    } ResourceType;
    
    virtual KURL getURL(ResourceType) = 0;
};

After this, I am planning to re-factor the Page object as - 

Page.h
======
class Page : public Supplementable<Page> {
public:
    struct PageClients {
    public:
        ResourceClient* resourceClient;
    };
    
    ResourceClient* resourceClient() const { return m_resourceClient; }

private:
    ResourceClient* m_resourceClient;
};

Then this would be consumed in the Internals object as 

Internals.cpp
=============
void Internals::openDummyInspectorFrontend()
{
    Page* page = contextDocument()->frame()->page();

    if (!page)
        return false;
        
    ResourceClient* resourceClient = page->resourceClient();
        
    ASSERT(resourceClient);

    Page::PageClients pc;
    fillWithEmptyClients(pc);

    m_frontendPage = adoptPtr(new Page(pc))
    
    ResourceRequest request(resourceClient->getURL(ResourceClient::INSPECTOR_PROTOCOL_URL)), false);        

    m_frontendPage->mainFrame()->loader()->load(request);
    
    ...
    
    return true;
}

So now the ports can implement this ResourceClient as

ResourceClientQt.h
==================
class ResourceClientQt : public ResourceClient
{
public:
    KURL getURL(ResourceType type);
};

ResourceClientQt.cpp
====================
KURL ResourceClientQt::getURL(ResourceType type)
{
    QUrl url;
    switch(type) {
    case INSPECTOR_URL:
         url = QUrl(QLatin1String("qrc:/webkit/inspector/inspector.html"));
         break;
    case INSPECTOR_PROTOCOL_URL:
         url = QUrl(QLatin1String("qrc:/webkit/inspector/inspector-protocol.html"));
         break;
    }
    return url;
}

This client can be installed during the creation of Page.

Please let me know your thoughts on this approach. 

I understand that this introduces a new client object in Page which might look like an overkill for just consuming in Internals object. But this would enable the WebCore seamlessly access the port specific resources in an elegant way.

[Hajime Morrita]

I respect inspector expert's opnion and here is just a thought but...
I feel that having another client interface is overkill as you think. 
Could it be just another InspectorClient API?
Having hard-coded filename in same file would be less error prone.

The client implementation could be optional and
we could implement it port-by-port.

[Yury Semikhatsky]

(In reply to comment #30)
> I respect inspector expert's opnion and here is just a thought but...
> I feel that having another client interface is overkill as you think. 
> Could it be just another InspectorClient API?
InspectorClient is only accessible from InspectorController and would require expanding its visibility so that it can be accessed from Internals.cpp

> Having hard-coded filename in same file would be less error prone.
> 
The URL is platform-dependent so we cannot just hard-code it.

> The client implementation could be optional and
> we could implement it port-by-port.
As I see the object creation is initiated by platform-dependent code where we know the inspector URL. What about passing the dummy front-end URL as a parameter to Internals object constructor(either directly or as a InternalsClient interface implementation)?

[Yury Semikhatsky]

(In reply to comment #29)
> As now we need to load the inspector-protocol.html in the newly created frontend page, I am thinking of following design approach. I would be glad to receive the feedback on the same.
> 
> I am planning to introduce a new client corresponding to Resource management. I am calling it a ResourceClient. 
> 
> ResourceClient.h
> ================
> class ResourceClient {    
> public:
>     typdef enum {
>         INSPECTOR_URL,
>         INSPECTOR_PROTOCOL_URL
The first type of resource is not used inside WebCore. It is a platform-specific thing which is handled in the WebKit or event embedder layer at the moment and I we shouldn't expose it to the WebCore without a good justification. The second type of resource is only needed in the Internals object so I'd think about a mechanism that would allow passing the URL to Internals object and not affect the code that is not used for the layout tests.

[Vivek Galatage]

(In reply to comment #31)
> (In reply to comment #30)
> > I respect inspector expert's opnion and here is just a thought but...
> > I feel that having another client interface is overkill as you think. 
> > Could it be just another InspectorClient API?
> InspectorClient is only accessible from InspectorController and would require expanding its visibility so that it can be accessed from Internals.cpp
> 
> > Having hard-coded filename in same file would be less error prone.
> > 
> The URL is platform-dependent so we cannot just hard-code it.
> 
> > The client implementation could be optional and
> > we could implement it port-by-port.
> As I see the object creation is initiated by platform-dependent code where we know the inspector URL. What about passing the dummy front-end URL as a parameter to Internals object constructor(either directly or as a InternalsClient interface implementation)?

Yeah this sounds promising. Yurys, can't we just access the page->inspectorController()->inspectorClient() and then access the newly introduced API from it? Or is this the same which you are referring to?

[Yury Semikhatsky]

(In reply to comment #33)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > I respect inspector expert's opnion and here is just a thought but...
> > > I feel that having another client interface is overkill as you think. 
> > > Could it be just another InspectorClient API?
> > InspectorClient is only accessible from InspectorController and would require expanding its visibility so that it can be accessed from Internals.cpp
> > 
> > > Having hard-coded filename in same file would be less error prone.
> > > 
> > The URL is platform-dependent so we cannot just hard-code it.
> > 
> > > The client implementation could be optional and
> > > we could implement it port-by-port.
> > As I see the object creation is initiated by platform-dependent code where we know the inspector URL. What about passing the dummy front-end URL as a parameter to Internals object constructor(either directly or as a InternalsClient interface implementation)?
> 
> Yeah this sounds promising. Yurys, can't we just access the page->inspectorController()->inspectorClient() and then access the newly introduced API from it? Or is this the same which you are referring to?

I wouldn't like to add methods that are used only by the testing harness to InspectorClient or Page->resourceClient()

[Vivek Galatage]

(In reply to comment #34)
> I wouldn't like to add methods that are used only by the testing harness to InspectorClient or Page->resourceClient()

Agreed. My apologies for not understanding it clearly before. 

How about following approach?

Introduce an InternalsClient.h in Source/WebCore/testing as:

InternalsClient.h
=================
class InternalsClient
{
public:
    virtual ~InternalsClient() {}
    virtual KURL inspectorProtocolURL() { return blankURL(); }
    // any further internals extension APIs which depends on port specific details can be added to this client object
};

extern PassOwnPtr<InternalsClient> provideInternalsClient();

This can be extended by the ports interested and the port specific clients can be placed in Source/WebCore/platform/[qt/gtk/...] (or will it be at Source/WebKit/[qt/gtk/...]/WebCoreSupport ?) with something like InternalsClientQt.h/cpp, InternalClientGtk.h/cpp etc.

In addition to this client, these ports must implement the provideInternalsClient() method.

Inside the Internals constructor, we can install this client and make use of it in our openDummyInspectorFrontend.

This way it can be used only within the testing harness. If we agree with this, should I create a separate bug and add the changes for the InternalsClient?

[Hajime Morrita]

Having InternalsClient would be nice.
But we could start from even simpler approach.
How about just add another Internals method and implement it in,
port-specific file like InternalsQt.cpp for example? 
If these port specific code grows, we can kick them out as a client.
But for now, we could start from simple if-defs.

----
// Internals.h
class Internals {
...
    KURL inspectorProtocolURL();
...
};

// Internals.cpp
#if !PLATFORM(QT)
KURL Internals::inspectorProtocolURL()
{
     return KURL();
}
#endif

// InternalsQt.cpp

KURL Internals::inspectorProtocolURL()
{
     return ...;
}
----

Anyway, thanks for taking time to explore different designs, Vivek.

[Vivek Galatage]

(In reply to comment #36)

Thank you Morrita for your comments.

> Having InternalsClient would be nice.
> But we could start from even simpler approach.
> How about just add another Internals method and implement it in,
> port-specific file like InternalsQt.cpp for example? 
> If these port specific code grows, we can kick them out as a client.
> But for now, we could start from simple if-defs.
> 
> ----
> // Internals.h
> class Internals {
> ...
>     KURL inspectorProtocolURL();
> ...
> };
> 
> // Internals.cpp
> #if !PLATFORM(QT)
> KURL Internals::inspectorProtocolURL()
> {
>      return KURL();
> }
> #endif
> 
> // InternalsQt.cpp
> 

So shall we place this port specific file, InternalsQt.cpp in Source/WebCore/platform/qt folder or some other place? 

> KURL Internals::inspectorProtocolURL()
> {
>      return ...;
> }
> ----
> 
> Anyway, thanks for taking time to explore different designs, Vivek.

[Hajime Morrita]

> 
> So shall we place this port specific file, InternalsQt.cpp in Source/WebCore/platform/qt folder or some other place? 
> 
For simplicity, WebCore/testing/ looks sufficient for me.

[Yury Semikhatsky]

(In reply to comment #38)
> > 
> > So shall we place this port specific file, InternalsQt.cpp in Source/WebCore/platform/qt folder or some other place? 
> > 
> For simplicity, WebCore/testing/ looks sufficient for me.

I think we will want to use the same platform-specific URL constant in the WebKit layer to avoid code duplication but WebCore/testing/ shouldn't have access to WebKit stuff except the client interfaces.

[Hajime Morrita]

(In reply to comment #39)
> (In reply to comment #38)
> > > 
> > > So shall we place this port specific file, InternalsQt.cpp in Source/WebCore/platform/qt folder or some other place? 
> > > 
> > For simplicity, WebCore/testing/ looks sufficient for me.
> 
> I think we will want to use the same platform-specific URL constant in the WebKit layer to avoid code duplication but WebCore/testing/ shouldn't have access to WebKit stuff except the client interfaces.
In that case, having InternalsClient is a way to go.

[Vivek Galatage]

I have created https://bugs.webkit.org/show_bug.cgi?id=92975 to track addition of InternalsClient.

[Vivek Galatage]

Discussion about which approach to take: 

https://bugs.webkit.org/show_bug.cgi?id=92975#c13

[Pavel Feldman]

Straw man: all tests will look like this:

<script src="harness.js"></script> /* common front-end harness code if needed */
<script>
function testBody()
{
    // A very long test body for this very test
}
window.internals.openDummyInspectorFrontend("(" + harnessBody.toString() + ")();" + "(" + testBody.toString() + ")();");
</script>

[Vivek Galatage]

(In reply to comment #43)
> Straw man: all tests will look like this:
> 
> <script src="harness.js"></script> /* common front-end harness code if needed */
> <script>
> function testBody()
> {
>     // A very long test body for this very test
> }
> window.internals.openDummyInspectorFrontend("(" + harnessBody.toString() + ")();" + "(" + testBody.toString() + ")();");
> </script>

We still need to evaluate the InspectorBackend.js and InspectorBackendCommands.js in the newly created page. So should this be part of the harness.js or we can refer to these files as is?

[Pavel Feldman]

I would keep injected script very small. By default, pure protocol tests should not necessarily need stubs for the protocol. If they do, they will import the harness file that does it for them.

[Pavel Feldman]

I think that harness.js (protocol-test.js) would inject following API into the test:

1) InspectorTest.sendCommand({string} method, {object} params, {function({string} error, {object}result} callback)

2) InspectorTest.setEventHandler(function(, {object} params)

And maybe more granular,

3) InspectorTest.addEventListener({string} method, {function({object} params)} callback)

As a start. The main idea is that (1) will conveniently bind request and response and invoke the callback.

[Pavel Feldman]

> 2) InspectorTest.setEventHandler(function(, {object} params)

missed the "method" param here. { } encode type here, i.e.

/**
  * @param {function({string} method, {object} params} handler
  */
InspectorTest.setEventHandler(handler)

[Vivek Galatage]

(In reply to comment #46)
> I think that harness.js (protocol-test.js) would inject following API into the test:
> 
> 1) InspectorTest.sendCommand({string} method, {object} params, {function({string} error, {object}result} callback)
> 

Should this method accept JSON string(method+args) directly for more raw representation? Otherwise we need to inherit(or copy) lot of code from InspectorBackend/Commands.js for converting them into proper JSON message format.

[Pavel Feldman]

(In reply to comment #48)
> (In reply to comment #46)
> > I think that harness.js (protocol-test.js) would inject following API into the test:
> > 
> > 1) InspectorTest.sendCommand({string} method, {object} params, {function({string} error, {object}result} callback)
> > 
> 
> Should this method accept JSON string(method+args) directly for more raw representation? Otherwise we need to inherit(or copy) lot of code from InspectorBackend/Commands.js for converting them into proper JSON message format.

I think it should accept method and params object as mentioned. It should not be hard to combine them into a message + JSON it.

[Vivek Galatage]

Created attachment 160123 [details]
Work in progress patch. Not ready for port specific changes.

[Yury Semikhatsky]

Comment on attachment 160123 [details]
Work in progress patch. Not ready for port specific changes.

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

> Source/WebCore/testing/Internals.cpp:1090
> +    page->inspectorController()->setInspectorFrontendClient(frontendClient.release());

frontendClient should be set on the m_frontendPage's InspectorController. Also you may need to add the following line to ensure that InspectorFrontendHost binding is properly setup since you use empty clients
 m_frontendPage->inspectorController()->didClearWindowObjectInWorld(mainFrame.get(), mainThreadNormalWorld());

[Vivek Galatage]

Comment on attachment 160123 [details]
Work in progress patch. Not ready for port specific changes.

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

>> Source/WebCore/testing/Internals.cpp:1090
>> +    page->inspectorController()->setInspectorFrontendClient(frontendClient.release());
> 
> frontendClient should be set on the m_frontendPage's InspectorController. Also you may need to add the following line to ensure that InspectorFrontendHost binding is properly setup since you use empty clients
>  m_frontendPage->inspectorController()->didClearWindowObjectInWorld(mainFrame.get(), mainThreadNormalWorld());

Thank you Yury for the feedback. 

I have seen if we set m_frontendPage->settings()->setScriptEnabled(true); then method didClearWindowObjectInWorld() gets called automatically at the time of evaluation. So which one should we use?

[Yury Semikhatsky]

(In reply to comment #52)
> (From update of attachment 160123 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160123&action=review
> 
> >> Source/WebCore/testing/Internals.cpp:1090
> >> +    page->inspectorController()->setInspectorFrontendClient(frontendClient.release());
> > 
> > frontendClient should be set on the m_frontendPage's InspectorController. Also you may need to add the following line to ensure that InspectorFrontendHost binding is properly setup since you use empty clients
> >  m_frontendPage->inspectorController()->didClearWindowObjectInWorld(mainFrame.get(), mainThreadNormalWorld());
> 
> Thank you Yury for the feedback. 
> 
> I have seen if we set m_frontendPage->settings()->setScriptEnabled(true); then method didClearWindowObjectInWorld() gets called automatically at the time of evaluation. So which one should we use?

Can you check whether the problem is that no setting m_frontendPage->settings()->setScriptEnabled(true); makes FrameLoader::dispatchDidClearWindowObjectInWorld return on canExecuteScripts check? If so then calling I'd prefer m_frontendPage->settings()->setScriptEnabled(true);

[Vivek Galatage]

> Can you check whether the problem is that no setting m_frontendPage->settings()->setScriptEnabled(true); makes FrameLoader::dispatchDidClearWindowObjectInWorld return on canExecuteScripts check? If so then calling I'd prefer m_frontendPage->settings()->setScriptEnabled(true);

You guessed it right. It returns on this condition. Hence I tried to set the flag and it worked. Sure then I will use this method. Thank you.

[Vivek Galatage]

Created attachment 160903 [details]
Patch

[Vivek Galatage]

Created attachment 160904 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160904 [details]
Patch

Attachment 160904 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13659079

New failing tests:
http/tests/inspector-protocol/testinternals.html

[WebKit Review Bot]

Created attachment 160912 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Yury Semikhatsky]

Comment on attachment 160904 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:1148
> +    m_frontendChannel.release();

You need to disconnect front-end first.

[Vivek Galatage]

Created attachment 160949 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 160949 [details]
Patch

Attachment 160949 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13656197

[WebKit Review Bot]

Comment on attachment 160949 [details]
Patch

Attachment 160949 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13665176

New failing tests:
http/tests/inspector-protocol/css-getSupportedCSSProperties.html

[WebKit Review Bot]

Created attachment 160955 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 160949 [details]
Patch

Attachment 160949 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13654576

[Yury Semikhatsky]

Comment on attachment 160949 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:118
> +    void attachWindow() { }

Although C++ doesn't require this, please preserve 'virtual' modifier on overriden virtual methods like we do in other places and also add OVERRIDE modifier to them.

> Source/WebCore/testing/Internals.cpp:1126
> +    static EmptyFrameLoaderClient* frameLoaderClient = adoptPtr(new EmptyFrameLoaderClient()).leakPtr();

I see that it is implemented this way in other places too but it would be nice to find an owner for the object instead of leaking it in a static variable. The owner would have to implemented FrameDestructionObserver I believe and would likely add more complexity to the code but I really don't like the fact that we need to create a leaking client here.

> LayoutTests/http/tests/inspector-protocol/css-getSupportedCSSProperties.html:1
> +<html>

This test should be under LayoutTests/inspector-protocol since it doesn't depend on running web server.

> LayoutTests/http/tests/inspector-protocol/css-getSupportedCSSProperties.html:9
> +        if (p1.name == p2.name)

== -> ===

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:27
> +    WebInspector = {};

Wrong alignment, should start from the the beginning of the line. See LayoutTests/http/tests/inspector/inspector-test.js for example.

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:46
> +    var message = "{\"method\": \"" + method + "\",\"params\":{" + parameters.join() + "},\"id\":"+ (this._id++) +"}";

I would create a JSON object and use JSON.stringify instead.

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:65
> +InspectorTest.setEventHandler = function(handler)

The method is unused. Please remove it and other unused.

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:83
> +    this._listeners[method] = handler;    

This looks like unnecessary complication. Why not use direct mapping requestId -> handler? Also can we reuse InspectorBackend.js + InspectorBackendCommands.js that we currently use in the front-end to parse and dispatch protocol messsages? The API should be generic enough to use it in the protocol test harness.

[Adam Barth]

Comment on attachment 160949 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:1120
> +    Page::PageClients pc;

pc <-- please use complete words in variable names.

> LayoutTests/http/tests/inspector-protocol/css-getSupportedCSSProperties-expected.txt:15
> +-webkit-animation-play-state
> +-webkit-animation-timing-function
> +-webkit-appearance
> +-webkit-aspect-ratio
> +-webkit-backface-visibility
> +-webkit-background-clip
> +-webkit-background-composite
> +-webkit-background-origin

Is this test going to need to be rebaselined every time we change which CSS properties we expose?  Can we make this test less fragile?

[Adam Barth]

Comment on attachment 160949 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:1121
> +    fillWithEmptyClients(pc);

As I've said before, I don't understand why we need to use EmptyClients to solve this problem.  EmptyClients are a giant hack that we shouldn't be using in new code.  In fact, I'd much rather see patches that remove the concept of EmptyClients.

[Adam Barth]

I don't understand why we can't just use a regular iframe.  I've asked before and no one can explain concretely what doesn't work.

[Adam Barth]

I should also say that it's likely I don't understand the constraints that have led to this design.  You shouldn't feel like you need to convince me.  vivekg and I were discussing this in #webkit just now and he asked me to post these comments to this bug.

[Yury Semikhatsky]

(In reply to comment #68)
> I don't understand why we can't just use a regular iframe.  I've asked before and no one can explain concretely what doesn't work.

(In reply to comment #69)
> I should also say that it's likely I don't understand the constraints that have led to this design.  You shouldn't feel like you need to convince me.  vivekg and I were discussing this in #webkit just now and he asked me to post these comments to this bug.

Main constrain here is that we have one InspectorController per Page. So we cannot use an iframe to host the front-end as it would share the same InspectorController instance while the front-end code is designed to use a separate InspectorController and Page. This is why we create a dedicated page which hosts the protocol client.

The framework is expected to be used for protocol tests only which means that it needs to be able to execute scripts and have a communication channel with the back-end. It shouldn't cause any network activity. Considering all of these we see that the framework can be implemented in WebCore without any platform-specific dependencies. Since the test doesn't require network support we create front-end page with EmtpyClients.

[Adam Barth]

If you need a new page, you can just use window.open to create one.

[Pavel Feldman]

> As I've said before, I don't understand why we need to use EmptyClients to solve this problem.  EmptyClients are a giant hack that we shouldn't be using in new code.  In fact, I'd much rather see patches that remove the concept of EmptyClients.

I've just added its usage in InspectorOverlay.cpp - I am using dummy page to paint the overlay (highlight) that we project over the main page later; I've seen several usages of it here:

http://code.google.com/p/chromium/source/search?q=fillWithEmptyClients&origq=fillWithEmptyClients&btnG=Search+Trunk

Why do we want to remove EmptyClients? If that concept is removed, can we have another way of creating page objects in the air?

[Yury Semikhatsky]

(In reply to comment #71)
> If you need a new page, you can just use window.open to create one.

(In reply to comment #71)
> If you need a new page, you can just use window.open to create one.

Yes, we missed that option. I believe we can try calling DOMWindow::open from openDummyInspectorFrontend instead of manually creating the front-end page. Rest of inspector plumbing should stay the same.

[Adam Barth]

> I've just added its usage in InspectorOverlay.cpp

Please stop adding uses of EmptyClients.  We want to reduce their usage and eventually remove the concept.  The more you add uses, the harder you make this task.
 
> http://code.google.com/p/chromium/source/search?q=fillWithEmptyClients&origq=fillWithEmptyClients&btnG=Search+Trunk

1) SVGImage is the hard nut to crack and the reason we haven't succeeded in removing EmptyClients yet.
2) PagePopup/PluginHelp (these are really the same).  We had an extensive discussion about whether to use EmtpyClients here and reluctantly decided to use them after weighing the current tradeoffs.

> Why do we want to remove EmptyClients?

Because it leads to crashes, memory leaks, and security vulnerabilities.  With EmptyClients, all code that uses clients (which is a lot of code!) needs to be aware that they might be talking to a dummy clients that don't behave in sane ways.

> If that concept is removed, can we have another way of creating page objects in the air?

It's hard to know how to answer that question since we haven't yet figured out how to implement SVG image without EmptyClients.  That's what's blocking their removal.

Please don't make this mess any worse.

[Vivek Galatage]

Created attachment 163579 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 163579 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:1130
> +    m_frontendWindow = adoptPtr(window->open(url, "", "", window, window).get());

No need to call .get() and then adoptPtr simply m_frontendWindow = window->open(url, "", "", window, window);

> Source/WebCore/testing/Internals.cpp:1138
> +    frontendPage->inspectorController()->setInspectorFrontendClient(m_frontendClient.release());

You should use a local variable to store the client here instead of m_frontendClient field since you clear m_frontendClient at this line.

> Source/WebCore/testing/Internals.cpp:1144
> +    frontendPage->mainFrame()->script()->evaluate(ScriptSourceCode(script));

Now that we create full-fledged window for the front-end, we could try to use window.postMessage to communicate with it.

> Source/WebCore/testing/Internals.cpp:1157
> +    m_frontendWindow->close(m_frontendWindow->scriptExecutionContext());

You should also call m_frontendWindow.clear().

> Source/WebCore/testing/Internals.h:232
> +    OwnPtr<DOMWindow> m_frontendWindow;

It must be RefPtr. We should probably fix OwnPtr to never take a pointer to a reference counted object.

[Yury Semikhatsky]

Comment on attachment 163579 [details]
Patch

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

> LayoutTests/http/tests/inspector-protocol/protocol-test.html:7
> +            InspectorBackend.loadFromJSONIfNeeded("../../../../Source/WebCore/inspector/Inspector.json");

Will it work for http tests? E.g. if LayoutTests/inspector-protocol/css-getSupportedCSSProperties.html were LayoutTests/http/tests/inspector-protocol/css-getSupportedCSSProperties.html

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:45
> +WebInspector.dispatchQueueIsEmpty = function() {

Unused method, remove it?

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:52
> +        InspectorBackend.dispatch(messagesToDispatch.shift());

Why not dispatch the messages synchronously? InspectorFrontendClientLocal::sendMessageToBackend is already asynchronous, isn't it enough?

> LayoutTests/http/tests/inspector-protocol/protocol-test.js:65
> +function log(text)

I'd extract this part into a separate file. Also can we reuse the printing code from inspector-test.js? It shouldn't depend on the inspector front-end.

> LayoutTests/inspector-protocol/css-getSupportedCSSProperties.html:16
> +        properties.sort(cssPropertySort);

No need to sort the properties anymore.

[Vivek Galatage]

Comment on attachment 163579 [details]
Patch

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

>> Source/WebCore/testing/Internals.cpp:1130
>> +    m_frontendWindow = adoptPtr(window->open(url, "", "", window, window).get());
> 
> No need to call .get() and then adoptPtr simply m_frontendWindow = window->open(url, "", "", window, window);

Sure, I will correct it.

>> Source/WebCore/testing/Internals.cpp:1138
>> +    frontendPage->inspectorController()->setInspectorFrontendClient(m_frontendClient.release());
> 
> You should use a local variable to store the client here instead of m_frontendClient field since you clear m_frontendClient at this line.

Agree.

>> Source/WebCore/testing/Internals.cpp:1144
>> +    frontendPage->mainFrame()->script()->evaluate(ScriptSourceCode(script));
> 
> Now that we create full-fledged window for the front-end, we could try to use window.postMessage to communicate with it.

Sure. Also can you explain what is the difference between the current method and postMessage as I am a new comer in this area.

>> Source/WebCore/testing/Internals.cpp:1157
>> +    m_frontendWindow->close(m_frontendWindow->scriptExecutionContext());
> 
> You should also call m_frontendWindow.clear().

Agree.

>> Source/WebCore/testing/Internals.h:232
>> +    OwnPtr<DOMWindow> m_frontendWindow;
> 
> It must be RefPtr. We should probably fix OwnPtr to never take a pointer to a reference counted object.

Sure.

>> LayoutTests/http/tests/inspector-protocol/protocol-test.html:7
>> +            InspectorBackend.loadFromJSONIfNeeded("../../../../Source/WebCore/inspector/Inspector.json");
> 
> Will it work for http tests? E.g. if LayoutTests/inspector-protocol/css-getSupportedCSSProperties.html were LayoutTests/http/tests/inspector-protocol/css-getSupportedCSSProperties.html

This is relative to the protocol-test.html. So as long as we keep this file at LayoutTests/http/tests/inspector-protocol directory, it will not be affected where the test case resides.

>> LayoutTests/http/tests/inspector-protocol/protocol-test.js:45
>> +WebInspector.dispatchQueueIsEmpty = function() {
> 
> Unused method, remove it?

Will remove it.

>> LayoutTests/http/tests/inspector-protocol/protocol-test.js:52
>> +        InspectorBackend.dispatch(messagesToDispatch.shift());
> 
> Why not dispatch the messages synchronously? InspectorFrontendClientLocal::sendMessageToBackend is already asynchronous, isn't it enough?

Yes this can sync. I borrowed these lines from the existing implementation. There must be specific reason there for making it async whereas we don't have any such specific requirement. I will correct it.

>> LayoutTests/http/tests/inspector-protocol/protocol-test.js:65
>> +function log(text)
> 
> I'd extract this part into a separate file. Also can we reuse the printing code from inspector-test.js? It shouldn't depend on the inspector front-end.

Can you please explain this comment. Couldn't quite get hold of this one.

>> LayoutTests/inspector-protocol/css-getSupportedCSSProperties.html:16
>> +        properties.sort(cssPropertySort);
> 
> No need to sort the properties anymore.

:) residual effect. I will remove it.

[Yury Semikhatsky]

(In reply to comment #78)
> (From update of attachment 163579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163579&action=review
> >> Source/WebCore/testing/Internals.cpp:1144
> >> +    frontendPage->mainFrame()->script()->evaluate(ScriptSourceCode(script));
> > 
> > Now that we create full-fledged window for the front-end, we could try to use window.postMessage to communicate with it.
> 
> Sure. Also can you explain what is the difference between the current method and postMessage as I am a new comer in this area.
> 
postMessage is a standard Web API. My point was that you could return the FE window from openDummyFrontend to the caller and the caller could later send messages to the FE via feWindow.postMessage. It will need to wait until the FE loading is complete but it is doable.


> >> LayoutTests/http/tests/inspector-protocol/protocol-test.html:7
> >> +            InspectorBackend.loadFromJSONIfNeeded("../../../../Source/WebCore/inspector/Inspector.json");
> > 
> > Will it work for http tests? E.g. if LayoutTests/inspector-protocol/css-getSupportedCSSProperties.html were LayoutTests/http/tests/inspector-protocol/css-getSupportedCSSProperties.html
> 
> This is relative to the protocol-test.html. So as long as we keep this file at LayoutTests/http/tests/inspector-protocol directory, it will not be affected where the test case resides.
> 
Source/WebCore/inspector/Inspector.json can be not server by web server in case of http tests. Please check that.

> >> LayoutTests/http/tests/inspector-protocol/protocol-test.js:52
> >> +        InspectorBackend.dispatch(messagesToDispatch.shift());
> > 
> > Why not dispatch the messages synchronously? InspectorFrontendClientLocal::sendMessageToBackend is already asynchronous, isn't it enough?
> 
> Yes this can sync. I borrowed these lines from the existing implementation. There must be specific reason there for making it async whereas we don't have any such specific requirement. I will correct it.
> 
We'll be able to add it later when we have a good reason for that. Otherwise it might be just another source of flakiness.


> >> LayoutTests/http/tests/inspector-protocol/protocol-test.js:65
> >> +function log(text)
> > 
> > I'd extract this part into a separate file. Also can we reuse the printing code from inspector-test.js? It shouldn't depend on the inspector front-end.
> 
> Can you please explain this comment. Couldn't quite get hold of this one.
> 
There are two suggestions:
1) Extract the code that should be executed in the test page into a separate file, no need to evaluate it in the FE page. It is based on the assumption that the FE .html file will link all necessary scripts(now the test code is passed into openDummyFrontend to be evaluated in the FE).
2) There is already some printing code in inspector-test.js would be great if we could reuse it here instead of adding another implementation.

[Vivek Galatage]

Created attachment 164528 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164528 [details]
Patch

Attachment 164528 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13890183

New failing tests:
inspector-protocol/css-getSupportedCSSProperties.html
http/tests/inspector-protocol/protocol-test.html

[Vivek Galatage]

Created attachment 164608 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 164608 [details]
Patch

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

> Source/WebCore/WebCore.exp.in:2235
> +__ZNK7WebCore9DOMWindow8documentEv

Please keep the entries sorted.

> Source/WebCore/inspector/test-front-end/protocol-test.html:2
> +Copyright (C) 2012 Samsung Electronics. All rights reserved.

To avoid running the protocol-test.html as a layout test you can put it under resources/ folder. So it would be something like LayoutTests/http/tests/inspector-protocol/resources/protocol-test.html

> Source/WebCore/inspector/test-front-end/protocol-test.html:30
> +window.addEventListener("message", function(event) {

This code can be moved to InspectorTest.js

Also we may need to send more messages from the inspected page to the front-end but I believe this function can be changed then.

> Source/WebCore/testing/Internals.cpp:1147
> +    m_frontendWindow->resetUnlessSuspendedForPageCache();

Is it really needed? I'd expect that m_frontendWindow->close should be enough.

> Source/WebCore/testing/Internals.cpp:1148
> +    m_frontendWindow->close(m_frontendWindow->scriptExecutionContext());

Also you should clear m_frontendWindow reference.

[Vivek Galatage]

(In reply to comment #83)
> (From update of attachment 164608 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164608&action=review
> > Source/WebCore/inspector/test-front-end/protocol-test.html:2
> > +Copyright (C) 2012 Samsung Electronics. All rights reserved.
> 
> To avoid running the protocol-test.html as a layout test you can put it under resources/ folder. So it would be something like LayoutTests/http/tests/inspector-protocol/resources/protocol-test.html
> 

I tried with the following entry in the httpd.conf file for resolving relative paths for the http tests.

RewriteRule ^/inspector-protocol(.*) %{DOCUMENT_ROOT}/../../../Source/WebCore/inspector$1

So after this we can just use the css-getSupportedCSSProperties.html as is just replacing the script source url as: 

<script src="http://localhost:8080/inspector-protocol/test-front-end/protocol-test.js"></script> 
             ************************************************************************

Now coming back to the file protocol-test.html which includes InspectorBackend.js (using the relative paths) and Inspector.json, if we keep the folder test-front-end inside the Source/WebCore/inspector folder, then it will work without any issues both for http and non-http tests. The resources as mentioned are relative urls, will be served properly even with the http requests.

But in case we move it to the location as mentioned above, the protocol-test.html must ensure it uses different version of the url paths which makes it little complicated. 

Your thoughts?

[Yury Semikhatsky]

(In reply to comment #84)
> (In reply to comment #83)
> > (From update of attachment 164608 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=164608&action=review
> > > Source/WebCore/inspector/test-front-end/protocol-test.html:2
> > > +Copyright (C) 2012 Samsung Electronics. All rights reserved.
> > 
> > To avoid running the protocol-test.html as a layout test you can put it under resources/ folder. So it would be something like LayoutTests/http/tests/inspector-protocol/resources/protocol-test.html
> > 
> 
> I tried with the following entry in the httpd.conf file for resolving relative paths for the http tests.
> 
> RewriteRule ^/inspector-protocol(.*) %{DOCUMENT_ROOT}/../../../Source/WebCore/inspector$1
> 
> So after this we can just use the css-getSupportedCSSProperties.html as is just replacing the script source url as: 
> 
> <script src="http://localhost:8080/inspector-protocol/test-front-end/protocol-test.js"></script> 
>              ************************************************************************
> 
> Now coming back to the file protocol-test.html which includes InspectorBackend.js (using the relative paths) and Inspector.json, if we keep the folder test-front-end inside the Source/WebCore/inspector folder, then it will work without any issues both for http and non-http tests. The resources as mentioned are relative urls, will be served properly even with the http requests.
> 
> But in case we move it to the location as mentioned above, the protocol-test.html must ensure it uses different version of the url paths which makes it little complicated. 
> 
> Your thoughts?
We discussed this with pfeldman and it seems that loading InspectorBackend.js and Inspector.json into protocol tests is getting unnecessarily convoluted. The test you added doesn't need any of these two files. That may well happen to be the case with other protocol tests and we won't need to load those files at all.

So at this point it would make sense to land bare minimum necessary to run the first protocol test. This would allow us to move forward. We will be able to add loading of InspectorBackend.js and Inspector.json later.

[Vivek Galatage]

(In reply to comment #85)
> We discussed this with pfeldman and it seems that loading InspectorBackend.js and Inspector.json into protocol tests is getting unnecessarily convoluted. The test you added doesn't need any of these two files. That may well happen to be the case with other protocol tests and we won't need to load those files at all.
> 
> So at this point it would make sense to land bare minimum necessary to run the first protocol test. This would allow us to move forward. We will be able to add loading of InspectorBackend.js and Inspector.json later.

Sure so we will go back to the implementation of sendMessage etc. as seen earlier to serialize/de-serialize the messages to and fro from the backend. That seems fine. As of this change then we wont even need to add any custom directives in the httpd.conf. The location mentioned "LayoutTests/http/tests/inspector-protocol/resources/" would work just fine. We had to do all the aliasing etc. in order to get the InspectorBackend.js and Inspector.json files from existing location.

[Yury Semikhatsky]

(In reply to comment #86)
> (In reply to comment #85)
> > We discussed this with pfeldman and it seems that loading InspectorBackend.js and Inspector.json into protocol tests is getting unnecessarily convoluted. The test you added doesn't need any of these two files. That may well happen to be the case with other protocol tests and we won't need to load those files at all.
> > 
> > So at this point it would make sense to land bare minimum necessary to run the first protocol test. This would allow us to move forward. We will be able to add loading of InspectorBackend.js and Inspector.json later.
> 
> Sure so we will go back to the implementation of sendMessage etc. as seen earlier to serialize/de-serialize the messages to and fro from the backend. That seems fine. As of this change then we wont even need to add any custom directives in the httpd.conf. The location mentioned "LayoutTests/http/tests/inspector-protocol/resources/" would work just fine. We had to do all the aliasing etc. in order to get the InspectorBackend.js and Inspector.json files from existing location.

Yes, if we don't use those files we don't need to bother with including them.

[Vivek Galatage]

Created attachment 165257 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 165257 [details]
Patch

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

> LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js:76
> +                        "command": { "name": "executeTest",

I think we can start with having only one command that would allow to eval arbitrary script in the front-end. Having name + params that consist of name and body look as overengineering too me at this stage. WDYT?

> LayoutTests/http/tests/inspector-protocol/resources/protocol-test.js:80
> +        inspectorFrontend.postMessage(JSON.stringify(message), "*");

No need to strigify/parse the message. Just pass it as an object.

[Vivek Galatage]

Created attachment 165335 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165335 [details]
Patch

Clearing flags on attachment: 165335

Committed r129346: <http://trac.webkit.org/changeset/129346>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #91)
> (From update of attachment 165335 [details])
> Clearing flags on attachment: 165335
> 
> Committed r129346: <http://trac.webkit.org/changeset/129346>

It broke the !ENABLE(INSPECTOR) build, see https://bugs.webkit.org/show_bug.cgi?id=97490 for details.