Closed Bug 1472718 Opened Last year Closed Last year

Convert ChromeUtils.requestIOActivity to a Promise

Categories

(Core :: Networking, enhancement, P5)

57 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: tarek, Assigned: tarek)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

Like in Bug 1471517 - let's convert this API to a promise.

This will be much more efficient to collect IOActivity than having notifications sent even if they are not used.

I suggest we also get rid of the timer+notification altogether. If needed, this is something that can be done on the JS side by using the ChromeUtil promise.
Assignee: nobody → tarek
Priority: -- → P2
Priority: P2 → P5
Whiteboard: [necko-triaged]
Comment on attachment 8989421 [details]
Bug 1472718 - Convert ChromeUtils.requestIOActivity to a Promise -

https://reviewboard.mozilla.org/r/254496/#review261300

::: dom/chrome-webidl/ChromeUtils.webidl:361
(Diff revision 2)
> +  Promise<sequence<IOActivityDataDictionnary>> requestIOActivity();
> +};
> +
> +
> +/**
> + * Used by requestIOActivity() to return the number of bytes 

extra space here.

::: netwerk/base/IOActivityMonitor.h:24
(Diff revision 2)
>  #include "private/pprio.h"
>  #include <stdint.h>
>  
> -namespace mozilla { namespace net {
> +namespace mozilla {
>  
> -#define IO_ACTIVITY_ENABLED_PREF "io.activity.enabled"
> +namespace dom { class Promise; }

same comment as the other patch.

::: netwerk/base/IOActivityMonitor.cpp:296
(Diff revision 2)
> -}
> -
>  // static
> -nsresult
> -IOActivityMonitor::NotifyActivities()
> +void
> +IOActivityMonitor::RequestActivities(dom::Promise* aPromise)
>  {

MOZ_ASSERT(aPromise)

::: netwerk/base/IOActivityMonitor.cpp:306
(Diff revision 2)
>    }
> -  return mon->NotifyActivities_Internal();
> +  mon->RequestActivitiesInternal(aPromise);
>  }
>  
> -nsresult
> -IOActivityMonitor::NotifyActivities_Internal()
> +void
> +IOActivityMonitor::RequestActivitiesInternal(dom::Promise* aPromise)

I would merge these 2 methods. but up to you

::: netwerk/base/IOActivityMonitor.cpp:323
(Diff revision 2)
> +        return;
> -    }
> +      }
> -  }
> +    }
> -  return NS_OK;
> -}
> +  }
> +  aPromise->MaybeResolve(activities);

I don't like that you resolve/reject promise inside the mutex. What about if you do:

nsresult result = NS_OK;
FallibleTArray<dom::IOActivityDataDictionnary> activities;

{
  mozilla::MutexAutoLock lock(mLock);

  // Remove inactive activities
  for (...
    if (NS_WARN_IF(!activities.Append...
      result = NS_ERROR_OUT_OF_MEMORY;
      break;
    ..
}

if (NS_WARN_IF(NS_FAILED(result))) {
  aPromise->MaybeReject(rv);
  return;
}

aPromise->MaybeResolve(activities);
Attachment #8989421 - Flags: review?(amarchesini) → review+
Comment on attachment 8989421 [details]
Bug 1472718 - Convert ChromeUtils.requestIOActivity to a Promise -

https://reviewboard.mozilla.org/r/254496/#review261294

Looks good.

::: dom/chrome-webidl/ChromeUtils.webidl:353
(Diff revision 1)
>     * Request performance metrics to the current process & all ontent processes.
>     */
>    void requestPerformanceMetrics();
>  
>    /**
> -  * Request IOActivityMonitor to send a notification containing I/O activity
> +  * Returns a Promise containing a sequence I/O activity

nit: sequence of I/O activities.

::: dom/chrome-webidl/ChromeUtils.webidl:361
(Diff revision 1)
> +  Promise<sequence<IOActivityDataDictionnary>> requestIOActivity();
> +};
> +
> +
> +/**
> + * Used by requestIOActivity() to return the number of bytes 

nit: trailing WS
Attachment #8989421 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8989421 [details]
Bug 1472718 - Convert ChromeUtils.requestIOActivity to a Promise -

https://reviewboard.mozilla.org/r/254494/#review261820
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c22aa9571139
Convert ChromeUtils.requestIOActivity to a Promise - r=baku,valentin
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8263446f439
Convert ChromeUtils.requestIOActivity to a Promise - r=baku,valentin
https://hg.mozilla.org/mozilla-central/rev/a8263446f439
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(tarek)
You need to log in before you can comment on or make changes to this bug.