Closed Bug 843865 Opened 9 years ago Closed 9 years ago

Networking Dashboard - implement using webidl

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: valentin, Assigned: valentin)

References

Details

Attachments

(2 files, 6 obsolete files)

Attached patch Patch 1.0 (obsolete) — Splinter Review
No description provided.
Attachment #716822 - Flags: review?(bzbarsky)
Comment on attachment 716822 [details] [diff] [review]
Patch 1.0

This seems to be missing some files?
Attached patch Patch 1.1 (obsolete) — Splinter Review
Ooops. My bad :)
I hope I've done it properly this time.
Attachment #716822 - Attachment is obsolete: true
Attachment #716822 - Flags: review?(bzbarsky)
Attachment #717009 - Flags: review?(bzbarsky)
Comment on attachment 717009 [details] [diff] [review]
Patch 1.1

OK, that's better.  ;)

You shouldn't need NetDashboard.h/cpp and the NetDashboard interface, if all you need is to force generation of those dictionaries.  Just add them to DummyInterface in dom/webidl/DummyBinding.webidl

>+++ b/dom/webidl/NetDashboard.webidl

>+  sequence<DOMString> host;

"hosts"?  Similar for other things in here, when it makess sense (e.g. for "active" or "tcp" it may not).

>+++ b/netwerk/base/src/Dashboard.cpp
>+    dict.mHost.Construct();
>+    dict.mPort.Construct();

Hmm.  This is a bit of a pain.  Sorry about that.  It's fine for now, but I'll think a bit about how we could make this more usable.

>+    Sequence< uint32_t > &ports =
>+        const_cast< Sequence< uint32_t >& >(dict.mPort.Value());

You shouldn't need the const_cast here and elsewhere, since dict is not const.  Please take that out.

>+        *hosts.AppendElement() = NS_ConvertASCIItoUTF16(mSock.data[i].host);

CopyASCIItoUTF16(mSock.data[i].host, *hosts.AppendElement());

and similar in some other places.

>+    dict.ToObject(cx, nullptr, &val);

  if (!dict.ToObject(cx, nullptr, &val)) {
    return NS_ERROR_FAILURE;
  }

and again similar in a few other places.

>-        for (uint32_t j = 0; j < mHttp.data[i].active.Length(); j++) {

So we used to store under "active" what looks like an array of objects object containing two arrays, right?  But now we just store an array of numbers?  Why the change?

There are several other places in this code with similar changes that don't make sense unless we're trying to change the output format.  r- for this part, unless there's a good reason for it.
Attachment #717009 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 717009 [details] [diff] [review]
> Patch 1.1
> 
> OK, that's better.  ;)
> 
> You shouldn't need NetDashboard.h/cpp and the NetDashboard interface, if all
> you need is to force generation of those dictionaries.  Just add them to
> DummyInterface in dom/webidl/DummyBinding.webidl
> 
> >+++ b/dom/webidl/NetDashboard.webidl
> 
> >+  sequence<DOMString> host;
> 
> "hosts"?  Similar for other things in here, when it makess sense (e.g. for
> "active" or "tcp" it may not).
> 
> >+++ b/netwerk/base/src/Dashboard.cpp
> >+    dict.mHost.Construct();
> >+    dict.mPort.Construct();
> 
> Hmm.  This is a bit of a pain.  Sorry about that.  It's fine for now, but
> I'll think a bit about how we could make this more usable.
> 
> >+    Sequence< uint32_t > &ports =
> >+        const_cast< Sequence< uint32_t >& >(dict.mPort.Value());
> 
> You shouldn't need the const_cast here and elsewhere, since dict is not
> const.  Please take that out.
> 
> >+        *hosts.AppendElement() = NS_ConvertASCIItoUTF16(mSock.data[i].host);
> 
> CopyASCIItoUTF16(mSock.data[i].host, *hosts.AppendElement());
> 
> and similar in some other places.
> 
> >+    dict.ToObject(cx, nullptr, &val);
> 
>   if (!dict.ToObject(cx, nullptr, &val)) {
>     return NS_ERROR_FAILURE;
>   }
> 
> and again similar in a few other places.
> 
> >-        for (uint32_t j = 0; j < mHttp.data[i].active.Length(); j++) {
> 
> So we used to store under "active" what looks like an array of objects
> object containing two arrays, right?  But now we just store an array of
> numbers?  Why the change?
> 
> There are several other places in this code with similar changes that don't
> make sense unless we're trying to change the output format.  r- for this
> part, unless there's a good reason for it.

Apparently webidl doesn't support sequence<sequence<anything>>: 
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#2241

On second thought, I think it might work if I use the any value in the webidl, and convert it to a JS::Val before saving it to the sequence.
(In reply to icecold.spam+bgz from comment #4)
That's why having multiple bugzilla accounts is a pain. :)
> Apparently webidl doesn't support sequence<sequence<anything>>: 

I'm going to try to just fix that in the next day or so.  But yes, in the meantime it could be hacked around with "any" if needed....  I'd rather we just do this right.
Depends on: 845666
Attached patch Patch 1.2 (obsolete) — Splinter Review
Sorry for the long break.
I need some feedback on the patch. It crashes for the sequence<sequence<nsString>> hostaddr, when calling .ToObject on the dictionary.

You can see the stack trace here: http://pastebin.mozilla.org/2257744
Attachment #717009 - Attachment is obsolete: true
Attachment #731317 - Flags: feedback?(bzbarsky)
Would you mind attaching the generated NetDashboardBinding.cpp too?
Attached file NetDashboardBinding.cpp v1.2 (obsolete) —
Attachment #731330 - Attachment mime type: text/x-c++src → text/plain
Thanks.  The codegen there is buggy. I'll file a bug and fix....
Thanks a lot!
Depends on: 856215
Comment on attachment 731317 [details] [diff] [review]
Patch 1.2

Would you happen to have an interdiff from the last thing I reviewed?
Attachment #731317 - Flags: feedback?(bzbarsky) → feedback+
Attached patch Patch 1.3 (obsolete) — Splinter Review
Sorry I can't provide an interdiff. The files have changed in the meantime.
Attachment #731317 - Attachment is obsolete: true
Attachment #731330 - Attachment is obsolete: true
Attachment #735344 - Flags: review?(bzbarsky)
Comment on attachment 735344 [details] [diff] [review]
Patch 1.3

The change was a pure search-and-replace with s/jsval/JS::Value/.  Should still be possible to apply the old patch to the tree after that s&r, then get an interdiff, I'd think.
Flags: needinfo?(valentin.gosu)
Attached patch interdiff (obsolete) — Splinter Review
Hope this is ok :)
Flags: needinfo?(valentin.gosu)
Yes, that's exactly what I was looking for.  Thank you!
Comment on attachment 735344 [details] [diff] [review]
Patch 1.3

>+    Sequence< uint32_t > &ports = dict.mPort.Value();

No need for the spaces around "uint32_t" and similar elsewhere in this file.  I know the bindings code has them, but that's because the bindings code is trying to deal with things like Sequence< Sequence<uint32_t> >, where at least the space in "> >" is in fact needed.

r=me with that nit.  Thank you again for the interdiff; it made this much simpler to review!
Attachment #735344 - Flags: review?(bzbarsky) → review+
Attached patch Patch 1.4Splinter Review
Final version
Attachment #735344 - Attachment is obsolete: true
Attachment #735938 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/11366920290d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
>+    Sequence<Sequence<nsString>> &hostaddr = dict.mHostaddr.Value();

This was a case that _did_ need need the extra spaces; see comment 17.  This patch as checked in breaks compilation with various compilers we actually still support.  Please fix!
Flags: needinfo?(valentin.gosu)
Attached patch Small FixSplinter Review
Fixed that little issue.
Attachment #737853 - Flags: review?(bzbarsky)
Flags: needinfo?(valentin.gosu)
Comment on attachment 737853 [details] [diff] [review]
Small Fix

Review of attachment 737853 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.
Attachment #737853 - Flags: review?(bzbarsky) → review+
Please checkin SmallFix
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Thanks!
https://hg.mozilla.org/mozilla-central/rev/ff35a0194d2b
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 865464
You need to log in before you can comment on or make changes to this bug.