Closed Bug 1428777 Opened 2 years ago Closed 2 years ago

Rename index files

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

Attachments

(1 file)

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
Priority: -- → P2
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).
(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
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
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:"....)
}
```
Attachment #8941059 - Flags: review?(gasolin)
(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 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
Attachment #8941059 - Flags: review?(gasolin) → review+
Removing the new initializer.js file from moz.build

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

Honza
https://hg.mozilla.org/mozilla-central/rev/606f0a737009
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.