catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 564 forks source link

Chrome-specific Event filtering methods should be moved to chrome_thread_helper.html #3460

Open eakuefner opened 7 years ago

eakuefner commented 7 years ago

We should move any methods that say "give me these events of interest" out of chrome_browser_helper after chrome_thread_helper is implemented (see #3458), and factor them to be of the form "give me a thread and I'll give you the events you want".

I think @benshayden had some other specific things that he wanted to see moved into chrome_thread_helper.

@zeptonaut @maxlgu

zeptonaut commented 7 years ago

One thing that's worthwhile note is that we've historically differentiated between things named ChromeThreadHelper (e.g. ChromeBrowserHelper) and files named like chrome_thread_helpers (e.g. iteration_helpers.html).

The former is an actual object that's created by passing it a thread, e.g.:

const helper = new ChromeThreadHelper(rendererHelper.mainThread);
return helper.getNetworkEvents();

The latter is just a file that contains (and exports) a whole bunch of helper functions:

<link rel="import" href="/tracing/model/helpers/chrome_thread_helpers.html">

<script>
...
function myMetric(model) {
  const thread = ...;
  const myNetworkEvents = tr.model.helpers.getThreadNetworkEvents(thread);
  ...
}
</script>

ChromeThreadHelper should probably use the former style, where it accepts a thread and then assumes that all operations are related to that thread.