| Summary: | New test inspector-protocol/page/archive.html added in r154828 fails on EFL, Qt, GTK | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zoltan Arvai <zarvai> | ||||||
| Component: | Web Inspector | Assignee: | Yury Semikhatsky <yurys> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | allan.jensen, commit-queue, hi, inspector-bugzilla-changes, joepeck, ossy, webkit-bug-importer, yurys | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 87008, 119774 | ||||||||
| Attachments: |
|
||||||||
|
Description
Zoltan Arvai
2013-09-04 08:04:51 PDT
Skipped on Qt in http://trac.webkit.org/changeset/155042. 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? Created attachment 382756 [details]
Patch
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 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 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 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 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 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. Created attachment 383145 [details]
Patch
Comment on attachment 383145 [details]
Patch
r=me
Comment on attachment 383145 [details] Patch Clearing flags on attachment: 383145 Committed r252305: <https://trac.webkit.org/changeset/252305> All reviewed patches have been landed. Closing bug. |