Bug 120682 - New test inspector-protocol/page/archive.html added in r154828 fails on EFL, Qt, GTK
Summary: New test inspector-protocol/page/archive.html added in r154828 fails on EFL, ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords: InRadar
Depends on:
Blocks: 87008 119774
  Show dependency treegraph
 
Reported: 2013-09-04 08:04 PDT by Zoltan Arvai
Modified: 2019-11-11 17:04 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.46 KB, patch)
2019-11-04 12:13 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2019-11-08 11:11 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Arvai 2013-09-04 08:04:51 PDT
Test fails with timeout or diff.

--- /mnt/buildbot/efl-linux-slave-1/efl-linux-64-release-wk1/build/layout-test-results/inspector-protocol/page/archive-expected.txt
+++ /mnt/buildbot/efl-linux-slave-1/efl-linux-64-release-wk1/build/layout-test-results/inspector-protocol/page/archive-actual.txt
@@ -1,2 +1,3 @@
-PASS: Received archive data.
+CONSOLE MESSAGE: line 4: TypeError: undefined is not an object (evaluating 'event.result.data')
+FAIL: Timed out waiting for notifyDone to be called
Comment 1 Zoltan Arvai 2013-09-04 08:38:00 PDT
Skipped on Qt in http://trac.webkit.org/changeset/155042.
Comment 2 Joseph Pecoraro 2013-09-04 13:38:46 PDT
My guess is this is because the port does not support ENABLE(WEB_ARCHIVE).

If that is the case, a minor tweak to the test can bail with the Page.archive(…) result error, and port specific results can be checked in for ports not implementing ENABLE(WEB_ARCHIVE).

Is there a page archive that these ports do support?
Comment 3 Yury Semikhatsky 2019-11-04 12:13:50 PST
Created attachment 382756 [details]
Patch
Comment 4 Devin Rousso 2019-11-04 12:37:08 PST
Comment on attachment 382756 [details]
Patch

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

> LayoutTests/platform/gtk/TestExpectations:-2331
> -webkit.org/b/120682 inspector/page/archive.html [ Timeout ]

If `Page.archive` isn't implemented (other than sending back "Not supported" as an error), why aren't we just skipping the test on these platforms?
Comment 5 Yury Semikhatsky 2019-11-04 13:05:30 PST
Comment on attachment 382756 [details]
Patch

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

>> LayoutTests/platform/gtk/TestExpectations:-2331
>> -webkit.org/b/120682 inspector/page/archive.html [ Timeout ]
> 
> If `Page.archive` isn't implemented (other than sending back "Not supported" as an error), why aren't we just skipping the test on these platforms?

It still may crash or timeout for various reasons and we don't want to have a protocol method that crashes even if the functionality is not implemented.
Comment 6 Devin Rousso 2019-11-07 15:15:05 PST
Comment on attachment 382756 [details]
Patch

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

>>> LayoutTests/platform/gtk/TestExpectations:-2331
>>> -webkit.org/b/120682 inspector/page/archive.html [ Timeout ]
>> 
>> If `Page.archive` isn't implemented (other than sending back "Not supported" as an error), why aren't we just skipping the test on these platforms?
> 
> It still may crash or timeout for various reasons and we don't want to have a protocol method that crashes even if the functionality is not implemented.

How exactly would this crash?  There are already general tests for protocol behavior.  If it's "Not supported", unless you yourself put something inside of it that causes a crash, it shouldn't ever cause a crash.

My concern with this is that it now requires any other ports that may want to add Web Inspector support to have to send back `errorString = "Not supported"_s;` instead of something that may be more descriptive/useful for that port.

If this truly isn't supported, then we should instead add support for `featureGuard` in the protocol JSON on a per-command/per-event basis (right now it can only be done per-domain), allowing you to describe what platforms/ports actually can do this.
Comment 7 Yury Semikhatsky 2019-11-07 15:31:47 PST
Comment on attachment 382756 [details]
Patch

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

>>>> LayoutTests/platform/gtk/TestExpectations:-2331
>>>> -webkit.org/b/120682 inspector/page/archive.html [ Timeout ]
>>> 
>>> If `Page.archive` isn't implemented (other than sending back "Not supported" as an error), why aren't we just skipping the test on these platforms?
>> 
>> It still may crash or timeout for various reasons and we don't want to have a protocol method that crashes even if the functionality is not implemented.
> 
> How exactly would this crash?  There are already general tests for protocol behavior.  If it's "Not supported", unless you yourself put something inside of it that causes a crash, it shouldn't ever cause a crash.
> 
> My concern with this is that it now requires any other ports that may want to add Web Inspector support to have to send back `errorString = "Not supported"_s;` instead of something that may be more descriptive/useful for that port.
> 
> If this truly isn't supported, then we should instead add support for `featureGuard` in the protocol JSON on a per-command/per-event basis (right now it can only be done per-domain), allowing you to describe what platforms/ports actually can do this.

Similar to any other test there is a possibility it may crash. Given that the crashes are rarely intentional I'd rather not speculate about how it can happen and have the tests catch it. This is also why we keep many tests marked as Failure instead of just skipping it.

If any port decides to change error message it can easily change the message either for other platforms too or just platform specific expectations.

Using guards would add even more fragmentation to the platforms and be more hassle to support. Ideally all commands should be the same across platforms unless inspected feature is platforms specific. In any case this is not the approach which is currently taken and it deserves a bigger discussion while this patch is cleaning up custom expectations and reduces time required to run the tests (Timeout always takes longest possible time to finish).
Comment 8 Devin Rousso 2019-11-07 16:45:29 PST
Comment on attachment 382756 [details]
Patch

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

>>>>> LayoutTests/platform/gtk/TestExpectations:-2331
>>>>> -webkit.org/b/120682 inspector/page/archive.html [ Timeout ]
>>>> 
>>>> If `Page.archive` isn't implemented (other than sending back "Not supported" as an error), why aren't we just skipping the test on these platforms?
>>> 
>>> It still may crash or timeout for various reasons and we don't want to have a protocol method that crashes even if the functionality is not implemented.
>> 
>> How exactly would this crash?  There are already general tests for protocol behavior.  If it's "Not supported", unless you yourself put something inside of it that causes a crash, it shouldn't ever cause a crash.
>> 
>> My concern with this is that it now requires any other ports that may want to add Web Inspector support to have to send back `errorString = "Not supported"_s;` instead of something that may be more descriptive/useful for that port.
>> 
>> If this truly isn't supported, then we should instead add support for `featureGuard` in the protocol JSON on a per-command/per-event basis (right now it can only be done per-domain), allowing you to describe what platforms/ports actually can do this.
> 
> Similar to any other test there is a possibility it may crash. Given that the crashes are rarely intentional I'd rather not speculate about how it can happen and have the tests catch it. This is also why we keep many tests marked as Failure instead of just skipping it.
> 
> If any port decides to change error message it can easily change the message either for other platforms too or just platform specific expectations.
> 
> Using guards would add even more fragmentation to the platforms and be more hassle to support. Ideally all commands should be the same across platforms unless inspected feature is platforms specific. In any case this is not the approach which is currently taken and it deserves a bigger discussion while this patch is cleaning up custom expectations and reduces time required to run the tests (Timeout always takes longest possible time to finish).

If another port/platform comes along, ideally they shouldn't have to change any test file to match their code, only test expectations.  As such, how about we change the test to just log the error message, and have GTKs expectations file contain that log so it passes with the expected error message?

LayoutTests/inspector/page/archive.html:
```
    InspectorProtocol.sendCommand("Page.archive", {}, function(event) {
        if (event.error) {
            ProtocolTest.fail(`Page.archive sent back error '${event.error}'.`);
            return;
        }

        ...
    });
```

LayoutTests/platform/gtk/inspector/page/archive-expected.txt"
```
FAIL: Page.archive sent back error 'Not supported'.
```

As an aside, I think this is exactly the kind of situation where we don't want the protocol to be the same across platforms.  Although `Page.archive` may not be platform specific in principle, in implementation it only actually works on platforms with `ENABLE_CF` and `ENABLE_WEB_ARCHIVE`.  What this means right now is that the frontend will show the archive button even on platforms where `Page.archive` is not supported, meaning that it will do nothing if any user clicks on it.  We shouldn't even be showing the button at all for platforms that don't implement its functionality.  The way Web Inspector has done this is to check whether protocol has that particular command/event exists or not, which can be controlled by build flags (as well as other things, like debuggableType/targetType as of <https://webkit.org/b/200384>).
Comment 9 Yury Semikhatsky 2019-11-08 11:11:01 PST
Comment on attachment 382756 [details]
Patch

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

>>>>>> LayoutTests/platform/gtk/TestExpectations:-2331
>>>>>> -webkit.org/b/120682 inspector/page/archive.html [ Timeout ]
>>>>> 
>>>>> If `Page.archive` isn't implemented (other than sending back "Not supported" as an error), why aren't we just skipping the test on these platforms?
>>>> 
>>>> It still may crash or timeout for various reasons and we don't want to have a protocol method that crashes even if the functionality is not implemented.
>>> 
>>> How exactly would this crash?  There are already general tests for protocol behavior.  If it's "Not supported", unless you yourself put something inside of it that causes a crash, it shouldn't ever cause a crash.
>>> 
>>> My concern with this is that it now requires any other ports that may want to add Web Inspector support to have to send back `errorString = "Not supported"_s;` instead of something that may be more descriptive/useful for that port.
>>> 
>>> If this truly isn't supported, then we should instead add support for `featureGuard` in the protocol JSON on a per-command/per-event basis (right now it can only be done per-domain), allowing you to describe what platforms/ports actually can do this.
>> 
>> Similar to any other test there is a possibility it may crash. Given that the crashes are rarely intentional I'd rather not speculate about how it can happen and have the tests catch it. This is also why we keep many tests marked as Failure instead of just skipping it.
>> 
>> If any port decides to change error message it can easily change the message either for other platforms too or just platform specific expectations.
>> 
>> Using guards would add even more fragmentation to the platforms and be more hassle to support. Ideally all commands should be the same across platforms unless inspected feature is platforms specific. In any case this is not the approach which is currently taken and it deserves a bigger discussion while this patch is cleaning up custom expectations and reduces time required to run the tests (Timeout always takes longest possible time to finish).
> 
> If another port/platform comes along, ideally they shouldn't have to change any test file to match their code, only test expectations.  As such, how about we change the test to just log the error message, and have GTKs expectations file contain that log so it passes with the expected error message?
> 
> LayoutTests/inspector/page/archive.html:
> ```
>     InspectorProtocol.sendCommand("Page.archive", {}, function(event) {
>         if (event.error) {
>             ProtocolTest.fail(`Page.archive sent back error '${event.error}'.`);
>             return;
>         }
> 
>         ...
>     });
> ```
> 
> LayoutTests/platform/gtk/inspector/page/archive-expected.txt"
> ```
> FAIL: Page.archive sent back error 'Not supported'.
> ```
> 
> As an aside, I think this is exactly the kind of situation where we don't want the protocol to be the same across platforms.  Although `Page.archive` may not be platform specific in principle, in implementation it only actually works on platforms with `ENABLE_CF` and `ENABLE_WEB_ARCHIVE`.  What this means right now is that the frontend will show the archive button even on platforms where `Page.archive` is not supported, meaning that it will do nothing if any user clicks on it.  We shouldn't even be showing the button at all for platforms that don't implement its functionality.  The way Web Inspector has done this is to check whether protocol has that particular command/event exists or not, which can be controlled by build flags (as well as other things, like debuggableType/targetType as of <https://webkit.org/b/200384>).

Just printing FAIL: followed by actual error sounds fine to me. The reason I used assert is just to make it clear for a reader unfamiliar with this code that the only expected error is 'Not implemented'. But it is only marginally better than just printing output, so I don't mind changing it that way. Updated the test.

As for 'archive' in particular, it could perhaps be more generic 'archive' in a platform-specific format. In any case there are very few platform-specific commands/features. If you don't want to show corresponding UI components you could add information about supported platforms to the protocol definition to hide the components on start.
Comment 10 Yury Semikhatsky 2019-11-08 11:11:21 PST
Created attachment 383145 [details]
Patch
Comment 11 Devin Rousso 2019-11-08 21:45:08 PST
Comment on attachment 383145 [details]
Patch

r=me
Comment 12 WebKit Commit Bot 2019-11-08 22:28:04 PST
Comment on attachment 383145 [details]
Patch

Clearing flags on attachment: 383145

Committed r252305: <https://trac.webkit.org/changeset/252305>
Comment 13 WebKit Commit Bot 2019-11-08 22:28:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2019-11-11 17:04:43 PST
<rdar://problem/57099549>