Closed Bug 1265813 Opened 4 years ago Closed 4 years ago

replace uses of nsIIOService

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file)

Replace uses of nsIIOService for devtools de-chrome-ification project.
Flags: qe-verify-
Priority: -- → P2
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 49.2 - May 23
Priority: P2 → P1
Attachment #8752339 - Flags: review?(pbrosset)
Comment on attachment 8752339 [details]
MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau

https://reviewboard.mozilla.org/r/52525/#review49720

I'd prefer :ochameau to review this, I'm not familiar with builtin-modules.
Having said this, this looks good.

I suppose there will be a part 2 patch where you replace the URL implementation?
Attachment #8752339 - Flags: review?(poirot.alex)
(In reply to Patrick Brosset <:pbro> from comment #2)
> Comment on attachment 8752339 [details]
> MozReview Request: Bug 1265813 - wrap devtools nsIIOService uses in URL
> shim; r?pbro
> 
> https://reviewboard.mozilla.org/r/52525/#review49720
> 
> I'd prefer :ochameau to review this, I'm not familiar with builtin-modules.
> Having said this, this looks good.
> 
> I suppose there will be a part 2 patch where you replace the URL
> implementation?

The idea is to make the devtools code "content-like".
Then whatever new content loader we come up with can simply use the
built-in URL and not provide a wrapper.
Comment on attachment 8752339 [details]
MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau

https://reviewboard.mozilla.org/r/52525/#review49722

(In reply to Tom Tromey :tromey from comment #3)
> (In reply to Patrick Brosset <:pbro> from comment #2)
> > Comment on attachment 8752339 [details]
> > MozReview Request: Bug 1265813 - wrap devtools nsIIOService uses in URL
> > shim; r?pbro
> > 
> > https://reviewboard.mozilla.org/r/52525/#review49720
> > 
> > I'd prefer :ochameau to review this, I'm not familiar with builtin-modules.
> > Having said this, this looks good.
> > 
> > I suppose there will be a part 2 patch where you replace the URL
> > implementation?
> 
> The idea is to make the devtools code "content-like".
> Then whatever new content loader we come up with can simply use the
> built-in URL and not provide a wrapper.

This patch doesn't help much. This is even quite confusing. It looks like we now expose the web URL constructor:
  https://developer.mozilla.org/en-US/docs/Web/API/URL/URL
But we aren't. We are still exposing the chrome interface, which is really different:
  https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIURI

We should be careful about what we put in builtin-modules.
Personally, I only accept thinks that matches existing web globals: atob, btoa, setTimeout, console, ...
"loader" and "isWorker" are really bad example and I'm against this practice.
It may be hard or impossible to expose these globals in es6 modules.
Ideally if we happen to switch to es6 modules, the whole builtin-modules will be dropped!

Also, it's not clear what are the next steps around nsIIOService,
this is just the tip of the iceberg. What are you planning for all the code that use these nsIURI objects?

A real step forward would be to expose the web URL constructor and convert code to use that instead!
But that sounds like a much harder refactoring and would expose the iceberg!

(You can get access to Web 'URL' constructor from builtin-modules with this: Cu.importGlobalProperties(["URL"]))
Attachment #8752339 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #4)

> Also, it's not clear what are the next steps around nsIIOService,
> this is just the tip of the iceberg. What are you planning for all the code
> that use these nsIURI objects?

I thought I got all of that code.
What did I miss?

> (You can get access to Web 'URL' constructor from builtin-modules with this:
> Cu.importGlobalProperties(["URL"]))

Ok, I can do that.
Funny, we seem to already define URL:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/builtin-modules.js#228

I hadn't noticed that before.
(In reply to Tom Tromey :tromey from comment #6)
> Funny, we seem to already define URL:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/builtin-
> modules.js#228
> 
> I hadn't noticed that before.

That should be the web one. Looks like using importGlobalProperties would be easier. I missed that while moving this code from Loader.jsm to a module. Cu.importGlobalProperties can only be used from a Sandbox scope.

(In reply to Tom Tromey :tromey from comment #5)
> (In reply to Alexandre Poirot [:ochameau] from comment #4)
> 
> > Also, it's not clear what are the next steps around nsIIOService,
> > this is just the tip of the iceberg. What are you planning for all the code
> > that use these nsIURI objects?
> 
> I thought I got all of that code.
> What did I miss?

Like various of those:
http://mxr.mozilla.org/mozilla-central/search?string=.newURI%28&find=%2Fdevtools%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
I think most of those hits don't need to be addressed in this patch; as IIUC this
project is really just trying to convert the inspector (sometimes we do more but
it's not a requirement).

We definitely don't need to convert server or the tests.
Comment on attachment 8752339 [details]
MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52525/diff/1-2/
Attachment #8752339 - Attachment description: MozReview Request: Bug 1265813 - wrap devtools nsIIOService uses in URL shim; r?pbro → MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r?ochameau
Attachment #8752339 - Flags: review?(poirot.alex)
This version exposes URL directly rather than via `require("URL")`; then fixes up the fallout.
It removes most uses of nsIIOService (one remains since it will be handled by some other
fix).  Finally, it replaces uses of sdk/url in shared code.
Comment on attachment 8752339 [details]
MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau

https://reviewboard.mozilla.org/r/52525/#review50140

Thanks, happy to see more code using Web features!

::: devtools/client/shared/components/reps/url.js:6
(Diff revision 2)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> -/* global URLSearchParams, URL */
> +/* global URLSearchParams */

Sorry for my eslint ignorance, but isn't it duplicate with your devtools/.eslintrc tweak?

::: devtools/client/shared/output-parser.js:650
(Diff revision 2)
>     *           - angleClass: ""         // The class to use for the angle value
>     *                                    // that follows the swatch.
>     *           - supportsColor: false   // Does the CSS property support colors?
>     *           - urlClass: ""           // The class to be used for url() links.
> -   *           - baseURI: ""            // A string or nsIURI used to resolve
> +   *           - baseURI: undefined     // A string or URL used to resolve
>     *                                    // relative links.

baseURI could be only a href string if text-property-editor.js
pass sheetHref and then you could also drop sheetURI from text-property-editor.js.

::: devtools/client/shared/source-utils.js:72
(Diff revision 2)
> -    let parsed = Object.assign({}, url, { host, fileName, hostname });
> -    gURLStore.set(location, parsed);
> -    return parsed;
> +      }
> +      if (fileName === "") {
> +        fileName = "/";
> +      }
> +    }
> +    url.fileName = fileName;

I really prefer this short version ;)
http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/utils/TabSources.js#321
  let filename = pathname.slice(pathname.lastIndexOf("/") + 1);
Unless it is buggy?
Attachment #8752339 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #11)

> > -/* global URLSearchParams, URL */
> > +/* global URLSearchParams */
> 
> Sorry for my eslint ignorance, but isn't it duplicate with your
> devtools/.eslintrc tweak?

The previous code declared "URL" as a global for this file; but the
new approach declares it as a global for all of devtools; so this patch
removes the file-specific global.

> baseURI could be only a href string if text-property-editor.js
> pass sheetHref and then you could also drop sheetURI from
> text-property-editor.js.

Nice find, thank you.  I'm doing this.

> I really prefer this short version ;)
> http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/utils/
> TabSources.js#321
>   let filename = pathname.slice(pathname.lastIndexOf("/") + 1);
> Unless it is buggy?

I'll give it a try.
Comment on attachment 8752339 [details]
MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52525/diff/2-3/
Attachment #8752339 - Attachment description: MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r?ochameau → MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau
Comment on attachment 8752339 [details]
MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52525/diff/3-4/
Comment on attachment 8752339 [details]
MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52525/diff/4-5/
Attachment #8752339 - Attachment description: MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau → MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r?ochameau
I think this needs another review, as I had to change a few things.
Sorry the interdiff is a mess, from rebasing.
The changes are:

* small tweak in gcli
* impedance match stuff in source-utils.js
* console-output.js:_isURL changed to filter for "useful" schemes
* worker loader makes URL available by default

Not sure how to re-request review on ReviewBoard so setting NI here.
Flags: needinfo?(poirot.alex)
https://reviewboard.mozilla.org/r/52525/#review51512

Looks good, thanks.

::: devtools/client/shared/source-utils.js:84
(Diff revision 5)
> -    let hostname = isChrome ? null : url.hostname;
> -    let host = isChrome ? null :
> -               url.port ? `${url.host}:${url.port}` :
> -               url.host;
> -
> -    let parsed = Object.assign({}, url, { host, fileName, hostname });
> +    if (url.pathname) {
> +      fileName = url.pathname.slice(url.pathname.lastIndexOf("/") + 1);
> +      if (fileName === "") {
> +        fileName = "/";
> +      }
> +    }

nit:
url.fileName = url.pathname ? url.pathname.slice(url.pathname.lastIndexOf("/") + 1) || "/";
Flags: needinfo?(poirot.alex)
Comment on attachment 8752339 [details]
MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52525/diff/5-6/
Attachment #8752339 - Attachment description: MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r?ochameau → MozReview Request: Bug 1265813 - replace nsIIOService with URL in devtools; r=ochameau
Keywords: checkin-needed
Iteration: 49.2 - May 23 → 49.3 - Jun 6
https://hg.mozilla.org/mozilla-central/rev/31267984105e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1276349
Depends on: 1277534
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.