1428777 - Rename index files

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The amount of JavaScript code in netmonitor/index.html increases and it would be nice to move it into separate JS file. We can't simply use index.js since it's used by the Launchpad.

So, what about:

1) index.js -> launchpad.js
2) index.html -> index.html & index.js

?

Honza

Comment 1

[[e-mail]]

I like `index.js -> launchpad.js` since its more clear.


But for `index.html -> index.html & index.js`, I think we'd better have somehow consistent naming convention with other devtool modules

* webconsole use `webconsole/new-console-output/main.js`
* performance-new use `performance-new/initializer.js`.

I suggest to use 

`index.html -> index.html & initializer.js`

to align with `performance-new/` and not confuse people who has experience on our old `index.js` (Since we likely left un-updated documents somewhere).

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1428777 - Rename index files;

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

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Fred Lin [:gasolin] from comment #1)
> I like `index.js -> launchpad.js` since its more clear.
Good, done (I also updated webpack.config.js)

> I suggest to use 
> 
> `index.html -> index.html & initializer.js`
> 
> to align with `performance-new/` and not confuse people who has experience
> on our old `index.js` (Since we likely left un-updated documents somewhere).
Agree, consistency is good and memory panel is using it too.

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941059 [details]
Bug 1428777 - Rename index files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211348/diff/1-2/

Comment 5

[[e-mail]]

Comment on attachment 8941059 [details]
Bug 1428777 - Rename index files;

https://reviewboard.mozilla.org/r/211348/#review217348

Thanks for the patch, I suggest to land getHar in a separate bug.

::: commit-message-6f5fa:3
(Diff revision 2)
> +Bug 1428777 - Rename index files; r=gasolin
> +
> +MozReview-Commit-ID: EI18B77FKJL

also need to update README.md 

`index.js` the entry point, equivalent to `index.html`. should be ->
`launchpad.js` the entry point, equivalent to `index.html`.

::: devtools/client/netmonitor/initializer.js:43
(Diff revision 2)
> +// Inject to global window for testing
> +window.store = store;
> +window.connector = connector;
> +
> +/**
> + * Global Netmonitor object in this panel. This object can be consumed

thanks for adding useful comments!

::: devtools/client/netmonitor/initializer.js:83
(Diff revision 2)
> +  },
> +
> +  /**
> +   * Returns list of requests currently available in the panel.
> +   */
> +  getHar() {

getHar looks like a new function, we can either land this in another bug or add a note in commit message.
I prefer to land in another bug so we can focus this bug on refactory.

::: devtools/client/netmonitor/initializer.js:84
(Diff revision 2)
> +
> +  /**
> +   * Returns list of requests currently available in the panel.
> +   */
> +  getHar() {
> +    let { HarExporter } = require("devtools/client/netmonitor/src/har/har-exporter");

Since it require an extra library when first loading, its worth to do a damp test and see if it effect the open time a lot.

::: devtools/client/netmonitor/initializer.js:166
(Diff revision 2)
> +      viewSourceInDebugger() {
> +        throw new Error("toolbox.viewSourceInDebugger is not implement from a tab");
> +      }
> +    };
> +    window.Netmonitor.bootstrap({ toolbox });
> +  })().catch(e => {

Since we use async function, instead of using promise catch, we can use normal `try..catch` sentense with await, like

```
try {
  let target = await targetFromURL(url);
  ...
  window.Netmonitor.bootstrap({ toolbox });
} catch (e) {
  window.alert("Unable to start the network monitor:"....)
}
```

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Fred Lin [:gasolin] from comment #5)
> also need to update README.md 
Done

> getHar looks like a new function
Ah, sure, not part of this patch, removed.

> Since we use async function, instead of using promise catch, we can use
> normal `try..catch` sentense with await, like
Done

Thanks!
Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941059 [details]
Bug 1428777 - Rename index files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211348/diff/2-3/

Comment 8

[[e-mail]]

Comment on attachment 8941059 [details]
Bug 1428777 - Rename index files;

https://reviewboard.mozilla.org/r/211348/#review217468

looks good, please make sure try is green before landing

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8941059 [details]
Bug 1428777 - Rename index files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211348/diff/3-4/

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Removing the new initializer.js file from moz.build

Try push looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d620ac79f3e8575d266a069ce507f5b2654b97f9

Honza

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/606f0a737009
Rename index files; r=gasolin

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/606f0a737009