WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
45286
Web Inspector: use string representation of resource type in extension API
https://bugs.webkit.org/show_bug.cgi?id=45286
Summary
Web Inspector: use string representation of resource type in extension API
Andrey Kosyakov
Reported
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).
Attachments
patch
(5.49 KB, patch)
2010-09-07 04:13 PDT
,
Andrey Kosyakov
no flags
Details
Formatted Diff
Diff
patch
(10.57 KB, patch)
2010-09-07 05:44 PDT
,
Andrey Kosyakov
no flags
Details
Formatted Diff
Diff
patch
(30.97 KB, patch)
2010-09-07 06:04 PDT
,
Andrey Kosyakov
yurys
: review-
Details
Formatted Diff
Diff
patch
(32.22 KB, patch)
2010-09-09 06:42 PDT
,
Andrey Kosyakov
yurys
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Andrey Kosyakov
Comment 1
2010-09-07 04:13:06 PDT
Created
attachment 66703
[details]
patch
Ilya Tikhonovsky
Comment 2
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.
Andrey Kosyakov
Comment 3
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.
Andrey Kosyakov
Comment 4
2010-09-07 06:04:04 PDT
Created
attachment 66712
[details]
patch Fixed ChangeLogs and binary files in the patch.
Yury Semikhatsky
Comment 5
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.
Andrey Kosyakov
Comment 6
2010-09-09 06:42:35 PDT
Created
attachment 67028
[details]
patch
Andrey Kosyakov
Comment 7
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.
WebKit Review Bot
Comment 8
2010-09-09 08:12:09 PDT
http://trac.webkit.org/changeset/67084
might have broken Qt Linux Release
Csaba Osztrogonác
Comment 9
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
Andrey Kosyakov
Comment 10
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
Csaba Osztrogonác
Comment 11
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.
Andrey Kosyakov
Comment 12
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 :)
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug