Tom Tromey :tromey Assignee
	Description • 8 years ago

Replace uses of nsIIOService for devtools de-chrome-ification project.

	Tom Tromey :tromey Assignee
	Comment 1 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/52525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52525/

	Patrick Brosset <:pbro>
	Comment 2 • 8 years ago

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?

	Tom Tromey :tromey Assignee
	Comment 3 • 8 years ago

(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.

	Alexandre Poirot [:ochameau]
	Comment 4 • 8 years ago

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"]))

	Tom Tromey :tromey Assignee
	Comment 5 • 8 years ago

(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.

	Tom Tromey :tromey Assignee
	Comment 6 • 8 years ago

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.

	Alexandre Poirot [:ochameau]
	Comment 7 • 8 years ago

(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

	Tom Tromey :tromey Assignee
	Comment 8 • 8 years ago

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.

	Tom Tromey :tromey Assignee
	Comment 9 • 8 years ago

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/

	Tom Tromey :tromey Assignee
	Comment 10 • 8 years ago

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.

	Alexandre Poirot [:ochameau]
	Comment 11 • 8 years ago

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?

	Tom Tromey :tromey Assignee
	Comment 12 • 8 years ago

(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.

	Tom Tromey :tromey Assignee
	Comment 13 • 8 years ago

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/

	Tom Tromey :tromey Assignee
	Comment 14 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c671fcb8c9a

	Tom Tromey :tromey Assignee
	Comment 15 • 8 years ago

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/

	Tom Tromey :tromey Assignee
	Comment 16 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c967466da3a

	Tom Tromey :tromey Assignee
	Comment 17 • 8 years ago

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/

	Tom Tromey :tromey Assignee
	Comment 18 • 8 years ago

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.

	Tom Tromey :tromey Assignee
	Comment 19 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=644570bf2870

	Alexandre Poirot [:ochameau]
	Comment 20 • 8 years ago

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) || "/";

	Tom Tromey :tromey Assignee
	Comment 21 • 8 years ago

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/

	Pulsebot
	Comment 22 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/31267984105e

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/31267984105e