Closed Bug 1309826 Opened 8 years ago Closed 7 years ago

convert netmonitor xul to xhtml

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Iteration:
54.2 - Feb 20
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: gasolin, Assigned: rickychien)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file, 1 obsolete file)

Bug 1309826 - convert netmonitor xul to xhtml
59 bytes, text/x-review-board-request
Honza
: review+
gasolin
: review+
Details 
once we have converted most xul elements to react components, we could convert netmonitor xul to xhtml format and make sure it works as usual
Flags: qe-verify?
Priority: -- → P2
Flags: qe-verify? → qe-verify-
When XUL is gone, several related tasks should be done. Either as part of this bug, or separately:

request-list-item.js component does some tricks with focus when a focused element is deleted. It's a workaround for XUL bugs. This won't be necessary in HTML-only UI.

Many UI elements still use XUL flex (-moz-box-*), although they are already HTML. This is needed for compatibility with the surrounding markup. Convert them all to standard flexbox.
Blocks: 1308441
Priority: P2 → P1
Blocks: 1308442
Priority: P1 → P2
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Iteration: --- → 54.2 - Feb 20
Priority: P2 → P1
First round review for de-XUL netmonitor patch! Please apply this patch of top of dependency patches.
Blocks: 1316291
Comment on attachment 8837415 [details]
Bug 1309826 - Convert MDN [learn more] link as a react component;

pushed to wrong bug, sorry for that
Attachment #8837415 - Flags: review?(odvarko)
Attachment #8837415 - Attachment is obsolete: true
Blocks: 1325730
Comment on attachment 8837086 [details]
Bug 1309826 - convert netmonitor xul to xhtml

https://reviewboard.mozilla.org/r/112336/#review114042

The flex layout makes custom request view's scrollbar and the requestMenu scrollbar works as normal, good job!

::: devtools/client/netmonitor/netmonitor.xhtml:26
(Diff revision 5)
> +      });
> +      window.windowRequire = require;
> +    </script>
> +  </head>
> +  <body class="theme-sidebar" role="application">
> +    <div class="mount"></div>

I prefer `root` which is a more common entrypoint, but the naming not block anything
Attachment #8837086 - Flags: review?(gasolin) → review+
`mount` is using in debugger but `root` is a good name for me :)
I can't apply the patch on m-c.

What are the dependency patches?
Do note that I have patch from bug 1309183 applied.

Honza
Flags: needinfo?(rchien)
Comment on attachment 8837415 [details]
Bug 1309826 - Convert MDN [learn more] link as a react component;

https://reviewboard.mozilla.org/r/112552/#review114122

Looks good, just one nit comment.

R+

Honza

::: devtools/client/netmonitor/shared/components/mdn-link.js:40
(Diff revision 1)
> +}
> +
> +MdnLink.displayName = "MdnLink";
> +
> +MdnLink.propTypes = {
> +  link: PropTypes.string,

Could we rename the prop to `url`?
Attachment #8837415 - Flags: review+
Flags: needinfo?(rchien)
Comment on attachment 8837415 [details]
Bug 1309826 - Convert MDN [learn more] link as a react component;

https://reviewboard.mozilla.org/r/112552/#review114150

Wrong patch attached.

Honza
Attachment #8837415 - Flags: review+
Comment on attachment 8837086 [details]
Bug 1309826 - convert netmonitor xul to xhtml

https://reviewboard.mozilla.org/r/112336/#review114182

lol

R+ assuming try is green

Honza

::: devtools/client/netmonitor/netmonitor-controller.js:37
(Diff revision 9)
>     * Initializes the view and connects the monitor client.
>     *
>     * @return object
>     *         A promise that is resolved when the monitor finishes startup.
>     */
> -  startupNetMonitor: Task.async(function* () {
> +  async startupNetMonitor() {

Again, this is a bit controversial. I am not going to block on this since the Debugger is already using it, but we should be ready to back out if needed.
Attachment #8837086 - Flags: review?(odvarko) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79bce9013f54
convert netmonitor xul to xhtml r=gasolin,Honza
https://hg.mozilla.org/mozilla-central/rev/79bce9013f54
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
This regressed the commonLibRequire trick jarda introduced in bug 1309866.
You should pass that to BrowserLoader() so that we prevent loading react module multiple times and uses toolbox one instead:
https://hg.mozilla.org/mozilla-central/rev/79bce9013f54#l9.19

It means that you can't create the browser loader from the xhtml file as toolbox is only passed later,
when the toolbox call inspector's panel object, here:
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/panel.js#7

panel.js is loaded by the toolbox, as a common sdk module. it isn't loaded as a browser module.
It then calls some netmonitor objects, like Netmonitor() on line 11.
Flags: needinfo?(rchien)
Flags: needinfo?(odvarko)
Thanks Alex for the pointer!

@Ricky: can you please file a new bug for this?

Honza
Flags: needinfo?(odvarko)
Thanks for reporting! Issue will be fixed in bug 1341159.
Flags: needinfo?(rchien)
Product: Firefox → DevTools
Regressions: 1585967
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: