Closed
Bug 1340366
Opened 7 years ago
Closed 7 years ago
Remove privilege APIs for har-builder, har-collector and clipboard
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox54 fixed)
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: rickychien, Assigned: rickychien)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor])
Attachments
(1 file)
Get rid of all XPCOM privilege APIs in order to make NetMonitor possible to run on content process.
Updated•7 years ago
|
Blocks: netmonitor-html
Flags: qe-verify?
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Iteration: --- → 54.2 - Feb 20
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8838360 [details] Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard https://reviewboard.mozilla.org/r/113292/#review114776 Thanks for the patch, some issue need to be addressed ::: devtools/client/netmonitor/har/har-builder.js:17 (Diff revision 4) > getFormDataSections, > getUrlQuery, > parseQueryString, > } = require("devtools/client/netmonitor/request-utils"); > > -loader.lazyGetter(this, "L10N", () => { > +const L10N = new LocalizationHelper("devtools/client/locales/har.properties"); please do so in `netmonitors/utils/l10n` for consistency, ``` const HAR_STRINGS_URI = "devtools/client/locales/har.properties"; exports.HAR_L10N = new LocalizationHelper(HAR_STRINGS_URI); ``` ::: devtools/client/netmonitor/har/har-collector.js:392 (Diff revision 4) > file.endedMillis = file.startedMillis + totalTime; > }, > > // Helpers > > - getLongHeaders: makeInfallible(function (headers) { > + getLongHeaders: function (headers) { Not sure if we should remove `makeInfallible` call, since debugger still use it somewhere so its fine to keep it
Attachment #8838360 -
Flags: review?(gasolin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8838360 [details] Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard https://reviewboard.mozilla.org/r/113292/#review114776 > please do so in `netmonitors/utils/l10n` for consistency, > > ``` > const HAR_STRINGS_URI = "devtools/client/locales/har.properties"; > exports.HAR_L10N = new LocalizationHelper(HAR_STRINGS_URI); > ``` I know there exists a netmonitors/utils/l10n but I'd prefer not to use it because har/ could treat as a separate module from netmonitor (it belongs to its folder). As a result, I'd like to leave that as it is. However, do you think har could be part of utils/ ? If that makes sense, we should move it to utils and merge l10n to utils/l10n as well. > Not sure if we should remove `makeInfallible` call, since debugger still use it somewhere so its fine to keep it It's merely a wrapper function for try catch and ignore any error.[1] It's safe. remove makeInfalliable can make us remove "devtools/shared/DevToolsUtils" as well. It would be easier to bring Netmonitor to Github since we don't have to care about bundling DevToolsUtils library (which is using a bunch of XPCOM APIs). [1] http://searchfox.org/mozilla-central/source/devtools/shared/ThreadSafeDevToolsUtils.js#98
Comment 8•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #7) > Comment on attachment 8838360 [details] > Bug 1340366 - Remove privilege APIs for har-builder, har-collector and > clipboard > > https://reviewboard.mozilla.org/r/113292/#review114776 > > > please do so in `netmonitors/utils/l10n` for consistency, > > > > ``` > > const HAR_STRINGS_URI = "devtools/client/locales/har.properties"; > > exports.HAR_L10N = new LocalizationHelper(HAR_STRINGS_URI); > > ``` > > I know there exists a netmonitors/utils/l10n but I'd prefer not to use it > because har/ could treat as a separate module from netmonitor (it belongs to > its folder). As a result, I'd like to leave that as it is. > > However, do you think har could be part of utils/ ? If that makes sense, we > should move it to utils and merge l10n to utils/l10n as well. I vote for keeping top level 'har' directory. I like the idea of treating HAR implementation as separate independent module. It's nice from architectural point of view but, I doubt that it could be use independently from the Network monitor. I think it's ok if the HAR module uses API from 'utils' (utils are supposed to be shared). Anyway, I am open for any suggestions and we can discuss at our meeting. > > > Not sure if we should remove `makeInfallible` call, since debugger still use it somewhere so its fine to keep it > > It's merely a wrapper function for try catch and ignore any error.[1] It's > safe. remove makeInfalliable can make us remove > "devtools/shared/DevToolsUtils" as well. It would be easier to bring > Netmonitor to Github since we don't have to care about bundling > DevToolsUtils library (which is using a bunch of XPCOM APIs). Sounds good to me. Honza > > [1] > http://searchfox.org/mozilla-central/source/devtools/shared/ > ThreadSafeDevToolsUtils.js#98
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8838360 [details] Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard https://reviewboard.mozilla.org/r/113292/#review114822 Since har-builder.js also used `request-utils` there's still some dependencies with netmonitor/, moving l10n to single reference might ease future tracing effort. But its fine to keep as it is
Attachment #8838360 -
Flags: review?(gasolin) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8838360 [details] Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard https://reviewboard.mozilla.org/r/113292/#review114920 R+, but please see my inline questions. Honza ::: devtools/client/netmonitor/har/har-collector.js:400 (Diff revision 5) > + try { > - this.getString(header.value).then(value => { > + this.getString(header.value).then(value => { > - header.value = value; > + header.value = value; > - }); > + }); > + } catch (err) { > + // Skip failures Shouldn't we have an error log here so, exceptions are not hidden? ::: devtools/client/netmonitor/netmonitor-controller.js:7 (Diff revision 5) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > -const promise = require("promise"); > const Services = require("Services"); Do we have a plan for `const Services = require("Services");` ?
Attachment #8838360 -
Flags: review?(odvarko) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #10) > ::: devtools/client/netmonitor/har/har-collector.js:400 > (Diff revision 5) > > + try { > > - this.getString(header.value).then(value => { > > + this.getString(header.value).then(value => { > > - header.value = value; > > + header.value = value; > > - }); > > + }); > > + } catch (err) { > > + // Skip failures > > Shouldn't we have an error log here so, exceptions are not hidden? Done. > > ::: devtools/client/netmonitor/netmonitor-controller.js:7 > (Diff revision 5) > > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > "use strict"; > > > > -const promise = require("promise"); > > const Services = require("Services"); > > Do we have a plan for `const Services = require("Services");` ? I think we can leverage Services shim from debugger.html, but I haven't investigated it yet. I'd like to ask Jason on IRC to see his feedback.
Comment 13•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3ef8779085ae Remove privilege APIs for har-builder, har-collector and clipboard r=gasolin,Honza
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3ef8779085ae
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Updated•7 years ago
|
Whiteboard: [netmonitor-reserve] → [netmonitor]
Updated•7 years ago
|
Flags: qe-verify? → qe-verify-
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•