Bug 45286

Summary: Web Inspector: use string representation of resource type in extension API
Product: WebKit Reporter: Andrey Kosyakov <caseq>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, loislo, ossy, pfeldman, webkit.review.bot, yurys, yutak
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
patch
none
patch
yurys: review-
patch yurys: review+

Description Andrey Kosyakov 2010-09-07 02:46:09 PDT
Extension API currently exposes resource types as numeric constants defined in WebCore::InspectorResource::Type. This is not safe, as types may be added in the middle of the list. We need to use string-based representation for resource types when exposing them in the extension API (e.g. "document", "font" etc).
Comment 1 Andrey Kosyakov 2010-09-07 04:13:06 PDT
Created attachment 66703 [details]
patch
Comment 2 Ilya Tikhonovsky 2010-09-07 04:55:41 PDT
Comment on attachment 66703 [details]
patch

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

> LayoutTests/inspector/extensions-events-expected.txt:12
> +        type : "other"
It'd be better to cover all other types too.
Comment 3 Andrey Kosyakov 2010-09-07 05:44:36 PDT
Created attachment 66707 [details]
patch

- Added tests for more resource types
- Split resource tests from main extension tests.
Comment 4 Andrey Kosyakov 2010-09-07 06:04:04 PDT
Created attachment 66712 [details]
patch

Fixed ChangeLogs and binary files in the patch.
Comment 5 Yury Semikhatsky 2010-09-08 07:05:28 PDT
Comment on attachment 66712 [details]
patch

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

> LayoutTests/inspector/extensions-resources.html:47
> +    webInspector.inspectedWindow.evaluate("var xhr = new XMLHttpRequest(); xhr.open('GET', '" + location.href + "', false); xhr.send(null);", callback);
This code can be extracted into a function on the inspected side and then you would only need to call that function. I guess it might make the code more clear.

> WebCore/inspector/front-end/ExtensionAPI.js:134
> +var ResourceTypes = {
Could we extract this into a file shared between ExtensionAPI.js and Resource.js, r- for this.
Comment 6 Andrey Kosyakov 2010-09-09 06:42:35 PDT
Created attachment 67028 [details]
patch
Comment 7 Andrey Kosyakov 2010-09-09 06:45:19 PDT
(In reply to comment #5)
> (From update of attachment 66712 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66712&action=prettypatch
> 
> > LayoutTests/inspector/extensions-resources.html:47
> > +    webInspector.inspectedWindow.evaluate("var xhr = new XMLHttpRequest(); xhr.open('GET', '" + location.href + "', false); xhr.send(null);", callback);
> This code can be extracted into a function on the inspected side and then you would only need to call that function. I guess it might make the code more clear.

Done.

> 
> > WebCore/inspector/front-end/ExtensionAPI.js:134
> > +var ResourceTypes = {
> Could we extract this into a file shared between ExtensionAPI.js and Resource.js, r- for this.

Done this as well, though I'm still not sure it's really better: the idea we duplicate resource type ids in the API is that those are actually our external interface and should remain same even if internal representation is changed. The automatic generation of ResourceTypes is more obscure, will work well as long as we only adding new resource type and will have to be rewritten if we ever rename a resource type.
Comment 8 WebKit Review Bot 2010-09-09 08:12:09 PDT
http://trac.webkit.org/changeset/67084 might have broken Qt Linux Release
Comment 9 Csaba Osztrogon√°c 2010-09-09 08:39:01 PDT
(In reply to comment #8)
> http://trac.webkit.org/changeset/67084 might have broken Qt Linux Release

The root of the problem is that Linux is case sensitive,
but MacOS and Windows isn't.

case sensitive compare: Ahem.ttf < abe.png  (because A<a)
case insensitive compare: abe.png < Ahem.ttf (because a==A and b<h)

I propose we should rename Ahem.ttf to ahem.ttf to avoid this problem.


--- /home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/layout-test-results/inspector/extensions-resources-expected.txt	2010-09-09 08:22:52.751418186 -0700
+++ /home/webkitbuildbot/slaves/release32bit/buildslave/qt-linux-release/build/layout-test-results/inspector/extensions-resources-actual.txt	2010-09-09 08:22:52.751418186 -0700
@@ -9,8 +9,8 @@
 resource: .../LayoutTests/inspector/extensions-resources.html, type: document
 resource: .../LayoutTests/inspector/extensions-resources.html, type: XHR
 resource: .../LayoutTests/inspector/extensions-test.js, type: script
-resource: .../inspector/resources/abe.png, type: image
 resource: .../inspector/resources/Ahem.ttf, type: font
+resource: .../inspector/resources/abe.png, type: image
 resource: .../inspector/resources/audits-style1.css, type: stylesheet
 resource: .../inspector/resources/missing-image.png, type: other
 RUNNING TEST: extension_testGetInvalidResource
Comment 10 Andrey Kosyakov 2010-09-09 08:53:58 PDT
Manually committed r67084: http://trac.webkit.org/changeset/67084
Linux test fixes committed as r67088: http://trac.webkit.org/changeset/67088
Comment 11 Csaba Osztrogon√°c 2010-09-09 08:55:39 PDT
(In reply to comment #10)
> Manually committed r67084: http://trac.webkit.org/changeset/67084
> Linux test fixes committed as r67088: http://trac.webkit.org/changeset/67088

Cool, thx for the fix.
Comment 12 Andrey Kosyakov 2010-09-09 08:56:57 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > http://trac.webkit.org/changeset/67084 might have broken Qt Linux Release
> 
> The root of the problem is that Linux is case sensitive,
> but MacOS and Windows isn't.
> 
> case sensitive compare: Ahem.ttf < abe.png  (because A<a)
> case insensitive compare: abe.png < Ahem.ttf (because a==A and b<h)
> 
> I propose we should rename Ahem.ttf to ahem.ttf to avoid this problem.

Thanks Csaba, I rather changed the sorting comparison function to ignore case -- less files to touch this way :)