Closed Bug 1469054 Opened 6 years ago Closed 6 years ago

Moving logic in current adb helper into devtools

Categories

(DevTools :: about:debugging, enhancement, P2)

Product:
DevTools
Component:
about:debugging
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Depends on 1 open bug)

Details

Attachments

(23 files, 4 obsolete files)

Bug 1469054 - Extract adb binary and related files into local profile directory from the extension.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - Adapt implementations for adb.start() and adb.stop() and relevant stuff.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
The difference between files from adb helper addon
6.76 KB, patch
Details | Diff | Splinter Review
Bug 1469054 - Make sure the adb server stops.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - ESLint: Lint fixes.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Tweaks for current about:debugging
3.49 KB, patch
Details | Diff | Splinter Review
Bug 1469054 - ESLint: Drop 'a' prefix from argument names.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - ESLint: Drop function name.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - ESLint: Use integer for the radix on parseInt.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - ESLint: Add no-fallthrough annotation.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - Add async annotation for adb.start().
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - Split Runtime.RuntimeTypes into a new file to import the RuntimeTypes in a file which is importing runtimes.js.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - Adapt scanner.js in the adbhelper addon into devtools.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - ESLint: lint fix for adb-scanner.js.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - Adapt device.js in the adbhelper addon into devtools.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - ESLint: lint fix for adb-device.js.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - Make sure that the adb.py sends all data.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
The new adb extension
4.17 MB, application/x-xpinstall
Details
Tweaks for WebIDE
3.73 KB, text/plain
Details
Bug 1469054 - Make sure the ADB server is ready to listen from clients when we call adb.start().
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
reduced_osx_test_case_tcpsocket.patch
6.80 KB, text/plain
Details
Bug 1469054 - Make check() in adb-running-checker fail if a certain period of time elapsed.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1469054 - Skip some test cases in test_adb.js on Windows.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
      No description provided.
Priority: -- → P2
I am going to push WIP patches as it is now.  Honestly I haven't recalled what works is left there. :)
Comment on attachment 8993163 [details]
Bug 1469054 - Extract adb binary and related files into local profile directory from the extension.

https://reviewboard.mozilla.org/r/257976/#review264862


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/aboutdebugging/initializer.js:28
(Diff revision 1)
>  });
>  
>  const { createFactory } = require("devtools/client/shared/vendor/react");
>  const { render, unmountComponentAtNode } = require("devtools/client/shared/vendor/react-dom");
>  const EventEmitter = require("devtools/shared/event-emitter");
> +const { getFileForBinary } = require("devtools/client/aboutdebugging/modules/adb-binary");

Error: 'getFileForBinary' is assigned a value but never used. [eslint: no-unused-vars]
FWIW, here is the diff between files moved from adbhelper addons.

This diff reminds me that I haven't decided how we should handle device-connected/device-disconnected events.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Attachment #8993166 - Attachment description: The difference between files in adb helper addon and → The difference between files from adb helper addon
So far, these changes seem reasonable to me! :) Thanks for posting the diff between the add-on, that helps a lot!
Here is an additional patch that I used for checking adbhelper functionalities works there.  I've checked that device connections/disconnections are properly observed, and the scanner is registered/unregisterd.

I am going to attach the new adbhelper extension that I've been using.
Attached file The new adbhelper extension (obsolete) — 
This extension includes binaries for all supported platforms for the convenience.
Hey, :jryans, these four uploaded patches are whole set of the patches for this bug.  I wanted to finish writing a unit test for each functionality here in this bug, but I think I can't finish it before your leave.  So I'd suggest to land these patches without tests and then in a later bug we (I) can finish the tests and do more works there (there is no UI in about:debugging yet, so we have to do more works anyway).  Any concerns?
Flags: needinfo?(jryans)
Comment on attachment 8993582 [details]
Bug 1469054 - Make sure the adb server stops.

https://reviewboard.mozilla.org/r/258288/#review265868

Looks reasonable to me.

::: devtools/client/aboutdebugging/modules/adb-scanner.js:9
(Diff revision 1)
> +
> +const EventEmitter = require("devtools/shared/event-emitter");
> +const { ConnectionManager } =
> +  require("devtools/shared/client/connection-manager");
> +const { Devices } =
> +  require("devtools/shared/apps/Devices.jsm");

It might make sense to remove the Devices.jsm intermedidate layer in the future.  It mostly exists as known module location to interact with add-ons that find devices.  Since that won't exist anymore, the Devices.jsm layer could be removed if desired.

::: devtools/client/aboutdebugging/modules/adb-scanner.js:12
(Diff revision 1)
> +  require("devtools/shared/client/connection-manager");
> +const { Devices } =
> +  require("devtools/shared/apps/Devices.jsm");
> +const Runtimes =
> +  // XXX: Tentative, runtimes should be also moved into aboutdebugging?
> +  require("devtools/client/webide/modules/runtimes");

Yes, I assume you would want to move them.
Comment on attachment 8993583 [details]
Bug 1469054 - ESLint: Lint fixes.

https://reviewboard.mozilla.org/r/258290/#review265874

Looks resonable to me.

::: devtools/client/aboutdebugging/modules/adb-device.js:16
(Diff revision 1)
> +/**
> + * A Device instance is created and registered with the Devices module whenever
> + * ADB notices a new device is connected.
> + *
> + * Any changes here should be examined carefully for backwards compatibility
> + * issues.  Other add-ons, like the Tools Adapter, make use of these low-level

No other add-ons these days...

::: devtools/client/aboutdebugging/modules/adb-device.js:29
(Diff revision 1)
> +  /**
> +   * DEPRECATED: This is specific to how we connect to Firefox OS.  Use cases
> +   * that interact with other kinds of devices should likely use the more
> +   * general |forwardPort| method directly.
> +   */
> +  connect(remotePort) {

Maybe it's time to delete this function if it's unused?
Good work, Hiro! :) Overall, it all seems reasonable to me.  Since I won't be involved in maintaining this code going forward, I think you and Julian should work whether to land now or wait for the tests.
Flags: needinfo?(jryans) → needinfo?(jdescottes)
Comment on attachment 8993163 [details]
Bug 1469054 - Extract adb binary and related files into local profile directory from the extension.

https://reviewboard.mozilla.org/r/257976/#review266612

Looks good! I have a few comments and questions. 

This patch fails ESLint in some spots (mainly usage of `let` where we enforce `const`) we should fix this before landing.

::: devtools/client/aboutdebugging/modules/adb-binary.js:18
(Diff revision 2)
> +                         "resource://gre/modules/NetUtil.jsm", true);
> +loader.lazyGetter(this, "UNPACKED_ROOT_PATH", () => {
> +  return OS.Path.join(OS.Constants.Path.localProfileDir, "adbhelper");
> +});
> +
> +const EXTENSION_ID = "adbhelper@mozilla.org";

Do we want to reuse the current adbhelper id? Maybe it would be useful to have a completely new extension here? This way we can keep updating the old extension for older firefox users still using WebIDE. What do you think?

::: devtools/client/aboutdebugging/modules/adb-binary.js:42
(Diff revision 2)
> +/**
> + * Unpack file from the extension.
> + * Uses NetUtil to read and write, since it's required for reading.
> + *
> + * @param {string} file
> + *        The base name of the file, such as "adb".

since we are doing file.split("/")[1] we probably expect something different from just "adb". The comment should reflect this.

::: devtools/client/aboutdebugging/modules/adb-binary.js:51
(Diff revision 2)
> +  if (!policy) {
> +    return;
> +  }
> +
> +  // Assumes that destination dir already exists.
> +  let filePath = OS.Path.join(UNPACKED_ROOT_PATH, file.split("/")[1]);

`file.split("/")[1]` -> this assumes strings will always contain one and only one "/". Either the comment for the param needs to be really explicit about that, or we should have a more generic approach to extract the filename from this path (ie, get the last part of the split)

::: devtools/client/aboutdebugging/modules/adb-binary.js:64
(Diff revision 2)
> +        // writing, rather than bouncing to OS.File...?
> +        let outputFile = new FileUtils.File(filePath);
> +        let output = FileUtils.openAtomicFileOutputStream(outputFile);
> +        NetUtil.asyncCopy(input, output, resolve);
> +      } catch (e) {
> +        console.log(`Could not unpack file ${file} in the extension: ${e}`);

Should we reject() here? What will happen to setPermissions if we failed to copy the file?

::: devtools/client/aboutdebugging/modules/adb-binary.js:74
(Diff revision 2)
> +  // Mark binaries as executable.
> +  await OS.File.setPermissions(filePath, { unixMode: 0o744 });
> +}
> +
> +/**
> + * Extract files in the extension into local profie directory and returns

nit: local profie -> local profile

::: devtools/client/aboutdebugging/modules/adb-binary.js:101
(Diff revision 2)
> +    //        "linux64/adb"
> +    //      ]
> +    //    },
> +    // ...
> +    filesForAdb =
> +      adbInfo[Services.appinfo.OS][Services.appinfo.XPCOMABI.split("-")[0]];

Can we add an example of String returned by Services.appinfo.XPCOMABI (eg `x86_64-gcc3`)

::: devtools/client/aboutdebugging/modules/adb-binary.js:121
(Diff revision 2)
> +  }
> +  return adbBinaryPath;
> +}
> +
> +/**
> + * Get a file object for the adb binary that was packed in this extension.

Maybe add some details about the extension:
- which extension is it?
- is it supposed to be already installed

::: devtools/client/aboutdebugging/test/test_adb.js:37
(Diff revision 2)
> +};
> +
> +ExtensionTestUtils.init(this);
> +
> +add_task(async function setup() {
> +  do_get_profile();

Can we add a comment to explain why this is needed?

::: devtools/client/aboutdebugging/test/test_adb.js:37
(Diff revision 2)
> +};
> +
> +ExtensionTestUtils.init(this);
> +
> +add_task(async function setup() {
> +  do_get_profile();

Our /test folder mixes mochitests and xpcshell tests. Consequently, eslint does not differentiate between one or the other and this test will be checked with the ESLint rules for mochitests.

This means that ESLint thinks do_get_profile is undefined here.

We should ultimately move the xpcshell tests to a separate folder so that we can have a dedicated eslintrc file for the xpcshell tests. In the meantime you can simply add `/* globals do_get_profile */` on top of the file.
Comment on attachment 8993164 [details]
Bug 1469054 - Adapt implementations for adb.start() and adb.stop() and relevant stuff.

https://reviewboard.mozilla.org/r/257978/#review266626

We should eslintignore the files copied from the addon for now and split + refactor them in a follow-up.

Either add `/* eslint-disable */` or add a reference to the file in .eslintignore.

Given the number of files maybe we could introduce a subfolder modules/adb/ here?
Ultimately I think adb/ might move to client/shared/ rather than living in aboutdebugging's folder so having things nicely separated would be good.

::: devtools/client/aboutdebugging/modules/adb-client.js:7
(Diff revision 2)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * A module to track device changes
> + * Mostly from original `adb.js`

Hard to know which adb.js this refers to :) 
For each of the files that are mostly copied from the current adbhelper addon, let's just say "adapted from adb-{name}.js at https://github.com/mozilla/adbhelper/tree/f44386c2d8cb7635a7d2c5a51191c89b886f8327 "
Comment on attachment 8993582 [details]
Bug 1469054 - Make sure the adb server stops.

https://reviewboard.mozilla.org/r/258288/#review265868

> Yes, I assume you would want to move them.

That depends, about:debugging will also have a notion of runtimes. Will we share them with ADB Helper or have a custom implementation? I am not sure. There will probably be a timeframe where this new ADBHelper has to work with both WebIDE and the new about:debugging.
Comment on attachment 8993583 [details]
Bug 1469054 - ESLint: Lint fixes.

https://reviewboard.mozilla.org/r/258290/#review266634

::: devtools/client/aboutdebugging/modules/adb-device.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Actually I can't find where this file is used?

Even in the current adbhelper addon, device.js is exposed as 
  defineLazyGetter(this, "Device", () => { return require("./device"); });

But I can't find where it is used in the addon or in mozilla-central?
Ryan, do you know if device.js is used with the current addon? (see comment above)
Flags: needinfo?(jdescottes) → needinfo?(jryans)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #21)
> Ryan, do you know if device.js is used with the current addon? (see comment
> above)

Ah, it relates to the following comment block[1] in the add-on:

/**
 * It may seem at first that registering devices with Device.jsm is no longer
 * needed, since they are now consumed in this same add-on in scanner.js (at
 * least for Firefox 36+ with new WebIDE API).  However, there are still use
 * cases for the Devices.jsm registry, as other add-ons can make use of these
 * low-level ADB devices if they know about other new runtimes to support on
 * the device.  This the approach used in the Valence add-on to find Chrome on
 * Android, for example.
 */

So, creating `Device` objects and registering them with `Devices.jsm` was already deprecated by the time of WebIDE.  We only continued to do it to support other add-ons from Firefox OS time that helped you update your phone, etc.  Since these are all now unsupported, the `Device` object / device.js and `Devices.jsm` registry can be removed.

[1]: https://github.com/mozilla/adbhelper/blob/364b1fc662134c361bed6de92d95e139dc0f8b59/main.js#L89-L97
Flags: needinfo?(jryans)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #17)
> ::: devtools/client/aboutdebugging/modules/adb-binary.js:18
> (Diff revision 2)
> > +                         "resource://gre/modules/NetUtil.jsm", true);
> > +loader.lazyGetter(this, "UNPACKED_ROOT_PATH", () => {
> > +  return OS.Path.join(OS.Constants.Path.localProfileDir, "adbhelper");
> > +});
> > +
> > +const EXTENSION_ID = "adbhelper@mozilla.org";
> 
> Do we want to reuse the current adbhelper id? Maybe it would be useful to
> have a completely new extension here? This way we can keep updating the old
> extension for older firefox users still using WebIDE. What do you think?

Oh right.  I had actually a brief chat with Brian on slack about relevant thing.  The topic was about the repository of the new extesion, but yeah we have to rename the id too.  adb@mozilla.org?

> ::: devtools/client/aboutdebugging/modules/adb-binary.js:64
> (Diff revision 2)
> > +        // writing, rather than bouncing to OS.File...?
> > +        let outputFile = new FileUtils.File(filePath);
> > +        let output = FileUtils.openAtomicFileOutputStream(outputFile);
> > +        NetUtil.asyncCopy(input, output, resolve);
> > +      } catch (e) {
> > +        console.log(`Could not unpack file ${file} in the extension: ${e}`);
> 
> Should we reject() here? What will happen to setPermissions if we failed to
> copy the file?

Oh right, indeed.  I will manage to write a test case for this.

> Can we add a comment to explain why this is needed?
> 
> ::: devtools/client/aboutdebugging/test/test_adb.js:37
> (Diff revision 2)
> > +};
> > +
> > +ExtensionTestUtils.init(this);
> > +
> > +add_task(async function setup() {
> > +  do_get_profile();
> 
> Our /test folder mixes mochitests and xpcshell tests. Consequently, eslint
> does not differentiate between one or the other and this test will be
> checked with the ESLint rules for mochitests.

Huh?  I didn't know that. :/

> This means that ESLint thinks do_get_profile is undefined here.
> 
> We should ultimately move the xpcshell tests to a separate folder so that we
> can have a dedicated eslintrc file for the xpcshell tests. In the meantime
> you can simply add `/* globals do_get_profile */` on top of the file.

Yeah it seems that we have to do that.

Thanks for the feedbacks!  I will address rest of comments.  Thanks!
Moving major about:debugging ng work into milestone 1, leaving m0 for prior bugfix work.
Blocks: remote-debugging-ng-m1
FWIW, I've noticed that there is a bug in the adbherlper extension.  Though I am not sure it's a serious, it might be one of the causes of connection failures.  Anyway I did notice it while writing test cases.

The adb binary can be run as a server and also as a client.  When there is no running adb server, the adbhelper starts a adb server, and stops the started server when 'adb-stop-polling' event [1] is received.  But this stopping is processed through client to the server on a TCP socket, and unfortunately we don't wait for the server finish properly.  So it's possible that the server is still alive for a while after the adbhelper thinks the server was stopped.  In a test case I wrote, we start an adb server soon after we stop another adb server which was triggered in a previous test.

It might be not a big problem in normal use cases, though.

[1] https://searchfox.org/mozilla-central/source/devtools/client/webide/modules/runtimes.js#207
I've added some more test cases, but I am not going to add any more test cases here in this bug.
I've succeeded to use the imported code from the adbhelper in the current WebIDE with some tweaks. I am going to post the tweak here in this but for the reference.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3282056d7eed0ec3cfdf6c23f949aee53bcc753
Oops, I forgot to mention about 'Device' object.  I am not going to drop the object in this bug, it's a very thin object, but it's still used for register/unregister adb devices through adb-scanner.js.  

Huh?  The test still fails on the try. :/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3282056d7eed0ec3cfdf6c23f949aee53bcc753&selectedJob=192190314
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #27)
> Oops, I forgot to mention about 'Device' object.  I am not going to drop the
> object in this bug, it's a very thin object, but it's still used for
> register/unregister adb devices through adb-scanner.js.  
> 
> Huh?  The test still fails on the try. :/
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d3282056d7eed0ec3cfdf6c23f949aee53bcc753&selectedJob=1
> 92190314

Sometimes we can't receive whole data set which was sent by adb.py at once.  We have to send the data as some chunks in adb.py.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08bd84f708a3b487c77044033bc42d063583a79
Attached patch Tweaks for WebIDE (obsolete) — — Splinter Review 
Here is the tweak to use the new logic in WebIDE.
Attached file The new adb extension — 
The extension id is now 'adb@mozilla.org'.
Attachment #8993586 - Attachment is obsolete: true
Attached file Tweaks for WebIDE — 
Julian told me on Slack that the tweaks for WebIDE doesn't work at all.  Yep, that's absolutely right.

Here is the correct patch for WebIDE.  Previously I did accidentally remove a patch in my MQ which is't actually needed for this bug, but it's needed for the WebIDE.

Sorry for confusing!
Attachment #8997848 - Attachment is obsolete: true
I confirm the new patch is working fine on my machine as well. Will proceed to review the patches now.
Comment on attachment 8993163 [details]
Bug 1469054 - Extract adb binary and related files into local profile directory from the extension.

https://reviewboard.mozilla.org/r/257976/#review268690

Looks good to me!

::: commit-message-5a810:3
(Diff revision 3)
> +Bug 1469054 - Extract adb binary and related files into local profile directory from the extension. r?jdescottes
> +
> +The new adbheler extension should have adb.json on the top of the extension

adbheler -> adbhelper (or replace with the new name, 'devtools-adb-extension' I think?)

::: devtools/shared/adb/adb-binary.js:18
(Diff revision 3)
> +                         "resource://gre/modules/NetUtil.jsm", true);
> +loader.lazyGetter(this, "UNPACKED_ROOT_PATH", () => {
> +  return OS.Path.join(OS.Constants.Path.localProfileDir, "adb");
> +});
> +
> +const EXTENSION_ID = "adb@mozilla.org";

Should we read this from the preference "devtools.webide.adbAddonID" instead?

::: devtools/shared/adb/adb-binary.js:140
(Diff revision 3)
> +async function getFileForBinary() {
> +  const path = await extractBinary();
> +  if (!path) {
> +    return null;
> +  }
> +  console.log(`Binary path: ${path}`);

So it seems that adbhelper was using a wrapper around console.log/debug/etc... (https://github.com/mozilla/adbhelper/blob/364b1fc662134c361bed6de92d95e139dc0f8b59/bootstrap.js#L100-L119)

Basically the logs only appeared if the preference "extensions.adbhelper@mozilla.org.debug" was true. 

I think it would be nice to make all the logs in shared/adb opt-in. There is one logging helper called dumpn in 
  `const { dumpn } = require("devtools/shared/DevToolsUtils");`
  
which logs only if "devtools.debugger.log" is true. Maybe use that?

Feel free to do this in a follow-up bug, or in a last changeset here. No need to update each commit in the series just to address this.

::: devtools/shared/moz.build:10
(Diff revision 3)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  include('../templates.mozbuild')
>  
>  DIRS += [
> +    'adb',

Let's keep this alphabetically sorted, so after 'acorn'. 

I didn't know that our build system was not forcing DIRS to be alphabetically sorted! Looking carefully I can see other subtle sorting issues in those array.
Attachment #8993163 - Flags: review?(jdescottes) → review+
Comment on attachment 8993164 [details]
Bug 1469054 - Adapt implementations for adb.start() and adb.stop() and relevant stuff.

https://reviewboard.mozilla.org/r/257978/#review268692

Works for me! A few things to fix before landing as well as suggestions for follow up bugs.

::: devtools/client/aboutdebugging/test/adb.py:1
(Diff revision 3)
> +#!/usr/bin/env python

This file was moved to devtools/shared and can be removed.

::: devtools/client/aboutdebugging/test/xpcshell.ini:6
(Diff revision 3)
> +support-files =
> +  adb.py

This is now in devtools/shared/adb and can be removed.

::: devtools/shared/adb/adb-client.js:7
(Diff revision 3)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * A module to track device changes
> + * Adpted from adb.js at

nit: adpted -> adapted

::: devtools/shared/adb/adb.js:9
(Diff revision 3)
> +// Whether or not this script is being loaded as a CommonJS module
> +// (from an add-on built using the Add-on SDK).  If it isn't a CommonJS Module,
> +// then it's a JavaScript Module.

This comment does not seem relevant anymore, we should remove it.

::: devtools/shared/adb/adb.js:17
(Diff revision 3)
> +
> +const { Cc, Ci, Cu } = require("chrome");
> +const EventEmitter = require("devtools/shared/event-emitter");
> +const client = require("./adb-client");
> +const { getFileForBinary } = require("./adb-binary");
> +const { setTimeout } = Cu.import("resource://gre/modules/Timer.jsm", {});

We are trying to get rid of Cu.import calls and instead use ChromeUtils.import (which should be exactly the same!).

We even had https://bugzilla.mozilla.org/show_bug.cgi?id=1440692 logged to do this in the addon. 

We can either fix it here or repurpose the bug above to fix the code imported here.

I think some of them can even be translated to require() calls. Maybe it will be better to do this in a cleanup follow up bug.

::: devtools/shared/adb/adb.js:19
(Diff revision 3)
> +const EventEmitter = require("devtools/shared/event-emitter");
> +const client = require("./adb-client");
> +const { getFileForBinary } = require("./adb-binary");
> +const { setTimeout } = Cu.import("resource://gre/modules/Timer.jsm", {});
> +const { Subprocess } = Cu.import("resource://gre/modules/Subprocess.jsm", {});
> +const { PromiseUtils } = Cu.import("resource://gre/modules/PromiseUtils.jsm", {});

Just as a reminder, we should check with :ochameau and see which promise implementation makes sense here.

::: devtools/shared/adb/adb.js:55
(Diff revision 3)
> +
> +  get adbFilePromise() {
> +    if (this._adbFilePromise) {
> +      return this._adbFilePromise;
> +    }
> +    this._adbFilePromise = getFileForBinary("adb");

getFileForBinary no longer takes an argument. The addon used to rely on this for extracting both adb and fastboot, but the new version is always extracting adb.

::: devtools/shared/adb/adb.js:81
(Diff revision 3)
> +      console.log("Didn't find ADB process running, restarting");
> +
> +      this.didRunInitially = true;
> +      let process = Cc["@mozilla.org/process/util;1"]
> +                      .createInstance(Ci.nsIProcess);
> +      let adbFile = await this.adbFilePromise;

The initial adb-binaries-manager had a logic to check if files were already unpacked (isUnpacked method).

The new adb-binary doesn't have this logic. And it looks like here we also don't check anything before asking for files to be unpacked. So every time we start the adb-scanner, if adb is nor running, the files are unpacked again.

I am fine with the change, but is it intentional? This optimization was not necessary?

::: devtools/shared/adb/adb.js:86
(Diff revision 3)
> +      let adbFile = await this.adbFilePromise;
> +      process.init(adbFile);
> +      // Hide command prompt window on Windows
> +      try {
> +        // startHidden is 55+
> +        process.startHidden = true;

we * could * probably remove this line but I would prefer to leave such cleanup for a follow up once we have landed everything and we are sure the new setup works fine.

Can we file a follow-up bug to "Remove code for old Firefox runtimes from devtools/shared/adb" ?

::: devtools/shared/adb/adb.js:1028
(Diff revision 3)
> +
> +    return deferred.promise;
> +  }
> +};
> +
> +exports.ADB = ADB;

Considering that this is an import from the addon I just made a superficial review of the code. We should have a follow-up to try to refactor this code a bit (1000 lines of JS is pretty long)

::: devtools/shared/adb/test/adb.py:21
(Diff revision 3)
> +import thread
> +
> +HOST = '127.0.0.1'
> +PORT = 5037
> +
> +class ADBServer(SocketServer.BaseRequestHandler):

thank you for adding this!

::: devtools/shared/adb/test/test_adb.js:154
(Diff revision 3)
> +add_task(async function testStartAndStop() {
> +  const extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      version: "1.0",
> +      applications: {
> +        gecko: { id: "adb@mozilla.org" }

I don't think we should read it from the preference here, but maybe a comment to indicate that this should match the default value for the preference?
Attachment #8993164 - Flags: review?(jdescottes) → review+
Comment on attachment 8993164 [details]
Bug 1469054 - Adapt implementations for adb.start() and adb.stop() and relevant stuff.

https://reviewboard.mozilla.org/r/257978/#review268706

Some additional comments about unused methods

::: devtools/shared/adb/adb.js:182
(Diff revision 3)
> +        }
> +      }, false);
> +    }
> +  },
> +
> +  async _isAdbRunning() {

This method is not used anywhere and can be removed.

::: devtools/shared/adb/adb.js:368
(Diff revision 3)
> +  },
> +
> +  // Checks a file mode.
> +  // aWhat is one the strings "S_ISDIR" "S_ISCHR" "S_ISBLK"
> +  // "S_ISREG" "S_ISFIFO" "S_ISLNK" "S_ISSOCK"
> +  checkFileMode: function adb_checkFileMode(aMode, aWhat) {

This method is not used and can be removed.

::: devtools/shared/adb/adb.js:410
(Diff revision 3)
> +  // send RECV + hex4(path.length) + path
> +  // while(needs data):
> +  //   recv DATA + hex4 + data
> +  // recv DONE + hex4(0)
> +  // send QUIT + hex4(0)
> +  pull: function adb_pull(aFrom, aDest) {

Not used in devtools, but was exposed in devices.js in the adbhelper extension? Will it be exposed later, do we want to keep this code around?

Same comment for push, shell, forwardPort, reboot etc...
Comment on attachment 8997847 [details]
Bug 1469054 - Make sure that the adb.py sends all data.

https://reviewboard.mozilla.org/r/261564/#review268716

The xpcshell test is still very intermittent on my machine. It often hangs on testStartAndStop, when connecting to ADB.
I investigated a bit, and the TCP socket is stuck on "connecting" state, and `onerror` is not called. Did you encounter this?

::: devtools/shared/adb/test/adb.py:27
(Diff revision 2)
> +        while sent_length < total_length:
> +            sent = self.request.send(all_data[sent_length:])
> +            sent_length = sent_length + sent

Can you add a small comment here to explain why we are doing this?
Comment on attachment 8993582 [details]
Bug 1469054 - Make sure the adb server stops.

https://reviewboard.mozilla.org/r/258288/#review268722

Clearing the flag here, because I have a question about the stop() method, and this seems like a good spot to discuss it:

Some things I noticed while reviewing this:
1- we are only stopping ADB from tests now?
2- we are no longer reacting to adb-start-polling and adb-stop-polling, is this intentional?
3- we are always calling stop with sync=true, should we remove the sync=false branch from the code? (note this is already the case in the current adb helper)

::: devtools/shared/adb/adb.js:122
(Diff revision 2)
>    async stop(sync) {
>      if (!this.didRunInitially) {
>        return; // We didn't start the server, nothing to do
>      }
>      await this.kill(sync);
> +    while (true) {

Can we add a comment explaining why we need to wait here? Since there is an `await` in front of this.kill, it is counter intuitive that adb is not killed when this.kill resolves.

::: devtools/shared/adb/adb.js:123
(Diff revision 2)
>      if (!this.didRunInitially) {
>        return; // We didn't start the server, nothing to do
>      }
>      await this.kill(sync);
> +    while (true) {
> +      const isAdbRunning = await require("./adb-running-checker").check();

This is the second spot where we require adb-running-checker here. We should move it to a require or lazy require on top of the file
Attachment #8993582 - Flags: review?(jdescottes)
Comment on attachment 8993583 [details]
Bug 1469054 - ESLint: Lint fixes.

https://reviewboard.mozilla.org/r/258290/#review268754
Attachment #8993583 - Flags: review?(jdescottes) → review+
Comment on attachment 8997837 [details]
Bug 1469054 - ESLint: Drop 'a' prefix from argument names.

https://reviewboard.mozilla.org/r/261544/#review268756
Attachment #8997837 - Flags: review?(jdescottes) → review+
Comment on attachment 8997838 [details]
Bug 1469054 - ESLint: Drop function name.

https://reviewboard.mozilla.org/r/261546/#review268758
Attachment #8997838 - Flags: review?(jdescottes) → review+
Comment on attachment 8997839 [details]
Bug 1469054 - ESLint: Use integer for the radix on parseInt.

https://reviewboard.mozilla.org/r/261548/#review268762

::: commit-message-7a5e2:3
(Diff revision 1)
> +Bug 1469054 - ESLint: Use integer for the radix on parseInt. r?jdescottes
> +
> +I am not sure this had caused real issue though.

Probably not :)
Attachment #8997839 - Flags: review?(jdescottes) → review+
Comment on attachment 8997840 [details]
Bug 1469054 - ESLint: Add no-fallthrough annotation.

https://reviewboard.mozilla.org/r/261550/#review268766

That's some really original code, but I agree we should not disturb it too much for now.
Attachment #8997840 - Flags: review?(jdescottes) → review+
Comment on attachment 8997841 [details]
Bug 1469054 - Add async annotation for adb.start().

https://reviewboard.mozilla.org/r/261552/#review268776
Attachment #8997841 - Flags: review?(jdescottes) → review+
Comment on attachment 8997841 [details]
Bug 1469054 - Add async annotation for adb.start().

https://reviewboard.mozilla.org/r/261552/#review268778
Comment on attachment 8997842 [details]
Bug 1469054 - Split Runtime.RuntimeTypes into a new file to import the RuntimeTypes in a file which is importing runtimes.js.

https://reviewboard.mozilla.org/r/261554/#review268780
Attachment #8997842 - Flags: review?(jdescottes) → review+
Comment on attachment 8997843 [details]
Bug 1469054 - Adapt scanner.js in the adbhelper addon into devtools.

https://reviewboard.mozilla.org/r/261556/#review268782

::: devtools/shared/adb/adb-scanner.js:103
(Diff revision 1)
> +  get id() {
> +    return this.device.id + "|" + this._socketPath;
> +  },
> +};
> +
> +function FirefoxOSRuntime(device, model) {

We should remove everything related to FirefoxOSRuntime, but a follow up bug would be ok here.
Attachment #8997843 - Flags: review?(jdescottes) → review+
Comment on attachment 8997844 [details]
Bug 1469054 - ESLint: lint fix for adb-scanner.js.

https://reviewboard.mozilla.org/r/261558/#review268784
Attachment #8997844 - Flags: review?(jdescottes) → review+
Comment on attachment 8997846 [details]
Bug 1469054 - ESLint: lint fix for adb-device.js.

https://reviewboard.mozilla.org/r/261562/#review268786
Attachment #8997846 - Flags: review?(jdescottes) → review+
Comment on attachment 8997842 [details]
Bug 1469054 - Split Runtime.RuntimeTypes into a new file to import the RuntimeTypes in a file which is importing runtimes.js.

https://reviewboard.mozilla.org/r/261554/#review268790

::: devtools/client/webide/modules/runtimes.js
(Diff revision 1)
>  
> -/* RUNTIMES */
> -
> -// These type strings are used for logging events to Telemetry.
> -// You must update Histograms.json if new types are added.
> -var RuntimeTypes = exports.RuntimeTypes = {

This might sound weird but we should keep exposing RuntimeTypes here:

exports.RuntimeTypes = RuntimeTypes;

The current adbhelper addon is relying on it and I think we plan the following sequence of events:
1/ Land this bug (and everything should still work, thah means we should be compatible with old adbhelper)
2/ Release the new extension 
3/ Update WebIDE to use the new ADB Helper

If we add this export, WebIDE still works as expected with the old adbhelper, and we can land this bug without any regression for users.
Since we are moving code, I am OK with landing this as long as :
- comment 67 is addressed
- intermittent failure mentioned in comment 54 investigated (I am testing on OSX). Or at least have a try push with OSX retriggers to make sure we will not get backed out

All the other comments can be moved to follow-ups and discussions. This is a lot of code moving around so I think that it would be good to land it early and then iterate on it together.
Comment on attachment 8997847 [details]
Bug 1469054 - Make sure that the adb.py sends all data.

https://reviewboard.mozilla.org/r/261564/#review268796
Attachment #8997847 - Flags: review?(jdescottes) → review+
Comment on attachment 8997845 [details]
Bug 1469054 - Adapt device.js in the adbhelper addon into devtools.

https://reviewboard.mozilla.org/r/261560/#review268798

In an earlier review I said push() pull() etc... were unused. 
Please ignore that comment, I don't why I didn't find the call sites here?
Attachment #8997845 - Flags: review?(jdescottes) → review+
Depends on: 1481691
Depends on: 1481699
(In reply to Julian Descottes [:jdescottes][:julian] from comment #67)
> > -/* RUNTIMES */
> > -
> > -// These type strings are used for logging events to Telemetry.
> > -// You must update Histograms.json if new types are added.
> > -var RuntimeTypes = exports.RuntimeTypes = {
> 
> This might sound weird but we should keep exposing RuntimeTypes here:
> 
> exports.RuntimeTypes = RuntimeTypes;
> 
> The current adbhelper addon is relying on it and I think we plan the
> following sequence of events:
> 1/ Land this bug (and everything should still work, thah means we should be
> compatible with old adbhelper)
> 2/ Release the new extension 
> 3/ Update WebIDE to use the new ADB Helper
> 
> If we add this export, WebIDE still works as expected with the old
> adbhelper, and we can land this bug without any regression for users.

This comment helped me a lot for future works that definitely happened in the near future. Thank you!
(In reply to Julian Descottes [:jdescottes][:julian] from comment #55)
> Comment on attachment 8993582 [details]
> Bug 1469054 - Make sure the adb server stops.
> 
> https://reviewboard.mozilla.org/r/258288/#review268722
> 
> Clearing the flag here, because I have a question about the stop() method,
> and this seems like a good spot to discuss it:
> 
> Some things I noticed while reviewing this:
> 1- we are only stopping ADB from tests now?

Yes, as of today.  I suppose we will do stop/start in the new about:debugging.

> 2- we are no longer reacting to adb-start-polling and adb-stop-polling, is
> this intentional?

Right, it's intentional.  As commented above, I think devtools controls stopping/starting adb.

> 3- we are always calling stop with sync=true, should we remove the
> sync=false branch from the code? (note this is already the case in the
> current adb helper)

Oh right, indeed, we should drop the argument.  I will do it in bug 1481691.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #68)
> Since we are moving code, I am OK with landing this as long as :
> - comment 67 is addressed
> - intermittent failure mentioned in comment 54 investigated (I am testing on
> OSX). Or at least have a try push with OSX retriggers to make sure we will
> not get backed out

So far, on Linux it gets more robust, but still it's very flaky on MacOSX.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d6bd2c9add37aebb90e16b043fdac929b9f5cb6&selectedJob=192719046

I have no idea yet why it's not flaky on Linux.

And there is another bug on Windows.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe5ee616873343a2891e285c5e61ce39de5ad382&selectedJob=192722500

I have fixed incorrectly handling the path string adb.json on Window, but still there is a problem. The problem happens on testNoTargetBinaries. I think the problem is that we don't remove extracted binaries in the profile when we remove the extension.  I don't know why it doesn't fail on other platforms.
The intermittent issue on OSX seems to be really low level. I will attach a reduced test case, but basically running:

  const socket = new TCPSocket("127.0.0.1", 5037, { binaryType: "arraybuffer" });
  await waitUntil(() => socket.readyState === "closed");

from a xpc-shell test, times out 25% of the time. The socket is stuck in readyState === "connecting". I tried turning off my network, changing the port ... no improvement.
To be applied on top of Hiro's current patches:

  hg pull -r 2009338ae996a8ee038d27cebe7a1fa124dce209 https://reviewboard-hg.mozilla.org/gecko
(In reply to Julian Descottes [:jdescottes][:julian] from comment #91)
> The intermittent issue on OSX seems to be really low level. I will attach a
> reduced test case, but basically running:
> 
>   const socket = new TCPSocket("127.0.0.1", 5037, { binaryType:
> "arraybuffer" });
>   await waitUntil(() => socket.readyState === "closed");
> 
> from a xpc-shell test, times out 25% of the time. The socket is stuck in
> readyState === "connecting". I tried turning off my network, changing the
> port ... no improvement.

Great!  Though I filed bug 1481963 for this issue, the test I added didn't fail on a try unfortunately. 

From IRC log:
jdescottes> I can't reproduce that from the browser console, so maybe it is xpcshell specific as well :/

I found there is a branch for content process [1], so yes, it's somewhat specific bugs on xpcshell.

[1] https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/dom/network/TCPSocket.cpp#237
As I left a comment in 1481963 comment 2, the issue on MacOSX is a long standing bug. I am going to introduce a timeout in the check function to avoid the stuck.  I hope it resolves the failures on MacOSX.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24649d9d8eac8534a66c7f9cb05c13c29de28f2f

So, the timeout hackaround works fine.  But still there are some test failures on Windows.  testNoTargetBinaries, testStartAndStop and testTrackDevices.  As for the last two I guess adb.py isn't launched properly on Windows.  Unfortunately I have no available Windows machine now, so I'd like to fix those tests in a later bug.
Comment on attachment 8998444 [details]
Bug 1469054 - Make sure the ADB server is ready to listen from clients when we call adb.start().

https://reviewboard.mozilla.org/r/261628/#review269042

Thanks Hiro! Some comments to address in the current implementation.

::: devtools/shared/adb/adb.js:69
(Diff revision 1)
> +        }
> +      }, false);
> +    });
> +  },
> +
> +  // Waits until a predicate returns true or re-tris the predicate calls

nit: re-tris -> retries

::: devtools/shared/adb/adb.js:70
(Diff revision 1)
> +      }, false);
> +    });
> +  },
> +
> +  // Waits until a predicate returns true or re-tris the predicate calls
> +  // |retry| times.

Maybe add that we try every 100ms, so the default value will wait for 2seconds at most.

::: devtools/shared/adb/adb.js:76
(Diff revision 1)
> +  async _waitUntil(predicate, retry = 20) {
> +    if (await predicate()) {
> +      return Promise.resolve(true);
> +    }
> +    if (retry-- <= 0) {
> +      Promise.resolve(false);

We should return here, and at line 80 we always resolve true. If I may suggest another implementation using less Promises:

  const waitUntil = async function(predicate, retry = 20) {
    let count = 0;
    while (count++ < retry) {
      if (await predicate()) {
        return true;
      }
      // Wait for 100 milliseconds.
      await new Promise(resolve => setTimeout(resolve, 100));
    }
    // Timed out after retrying too many times
    return false;
  }
  
This is an adaptation from asyncWaitUntil in shared-head.js. Not using Promises & recursion makes it easier to understand in my opinion

::: devtools/shared/adb/adb.js:119
(Diff revision 1)
> -      const self = this;
> -      process.runAsync(params, params.length, {
> +      await this._runProcess(process, params).then(async () => {
> +        if (await this._waitUntil(check)) {
> -        observe(subject, topic, data) {
> -          switch (topic) {
> -            case "process-finished":
> -              onSuccessfulStart();
> +          onSuccessfulStart();
> -              break;
> -            case "process-failed":
> +        } else {
> +          this.ready = false;
> -              self.ready = false;
> -              reject();
> -              break;
> -          }
>          }
> -      }, false);
> +      }, () => {
> +        this.ready = false;
> +      });
>      });

We no longer reject here? I would prefer to avoid then/promises for new code, especially if we are mixing it with async:

  let isStarted = false;
  try {
    await this._runProcess(process, params);
    isStarted = await this._waitUntil(check);
  } catch (e) {
    // runProcess failed
  }
  
  if (isStarted) {
    onSuccessfulStart();
  } else {
    this.ready = false;
    reject();
  }
Attachment #8998444 - Flags: review?(jdescottes)
Comment on attachment 8998714 [details]
Bug 1469054 - Skip some test cases in test_adb.js on Windows.

https://reviewboard.mozilla.org/r/261640/#review269046

::: devtools/shared/adb/test/test_adb.js:104
(Diff revision 1)
>  
>    await extension.unload();
>  });
>  
> -add_task(async function testNoTargetBinaries() {
> +add_task({
> +  skip_if: () => mozinfo.os == "win"

If you already have a bug logged to investigate those windows issues, can you mention it in a comment here?
Attachment #8998714 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes][:julian] from comment #114)
> Comment on attachment 8998444 [details]
> Bug 1469054 - Make sure the ADB server is ready to listen from clients when
> we call adb.start().
>
> ::: devtools/shared/adb/adb.js:70
> (Diff revision 1)
> > +      }, false);
> > +    });
> > +  },
> > +
> > +  // Waits until a predicate returns true or re-tris the predicate calls
> > +  // |retry| times.
> 
> Maybe add that we try every 100ms, so the default value will wait for
> 2seconds at most.
> 

I realize that it is actually 2 seconds + the time waiting on the predicate... A better comment would be to say we wait 100ms between each attempt then :)
Depends on: 1482008
(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #73)
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #55)
> > Comment on attachment 8993582 [details]
> > Bug 1469054 - Make sure the adb server stops.
> > 
> > https://reviewboard.mozilla.org/r/258288/#review268722
> > 
> > Clearing the flag here, because I have a question about the stop() method,
> > and this seems like a good spot to discuss it:
> > 
> > Some things I noticed while reviewing this:
> > 1- we are only stopping ADB from tests now?
> 
> Yes, as of today.  I suppose we will do stop/start in the new
> about:debugging.
> 
> > 2- we are no longer reacting to adb-start-polling and adb-stop-polling, is
> > this intentional?
> 
> Right, it's intentional.  As commented above, I think devtools controls
> stopping/starting adb.
> 

Answering here before I forget: this new ADB will be used by both the new about:debugging and by WebIDE. The main priority is to get it to work with WebIDE in release 63. So we will need to start/stop ADB from WebIDE as well. This was the role of the LazyADB scanner I think. But we can address that when we reach step 3 of my comment 67 :)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #114)
> ::: devtools/shared/adb/adb.js:76
> (Diff revision 1)
> > +  async _waitUntil(predicate, retry = 20) {
> > +    if (await predicate()) {
> > +      return Promise.resolve(true);
> > +    }
> > +    if (retry-- <= 0) {
> > +      Promise.resolve(false);
> 
> We should return here, and at line 80 we always resolve true. If I may
> suggest another implementation using less Promises:
> 
>   const waitUntil = async function(predicate, retry = 20) {
>     let count = 0;
>     while (count++ < retry) {
>       if (await predicate()) {
>         return true;
>       }
>       // Wait for 100 milliseconds.
>       await new Promise(resolve => setTimeout(resolve, 100));
>     }
>     // Timed out after retrying too many times
>     return false;
>   }
>   
> This is an adaptation from asyncWaitUntil in shared-head.js. Not using
> Promises & recursion makes it easier to understand in my opinion
> 
> ::: devtools/shared/adb/adb.js:119
> (Diff revision 1)
> > -      const self = this;
> > -      process.runAsync(params, params.length, {
> > +      await this._runProcess(process, params).then(async () => {
> > +        if (await this._waitUntil(check)) {
> > -        observe(subject, topic, data) {
> > -          switch (topic) {
> > -            case "process-finished":
> > -              onSuccessfulStart();
> > +          onSuccessfulStart();
> > -              break;
> > -            case "process-failed":
> > +        } else {
> > +          this.ready = false;
> > -              self.ready = false;
> > -              reject();
> > -              break;
> > -          }
> >          }
> > -      }, false);
> > +      }, () => {
> > +        this.ready = false;
> > +      });
> >      });
> 
> We no longer reject here? I would prefer to avoid then/promises for new
> code, especially if we are mixing it with async:
> 
>   let isStarted = false;
>   try {
>     await this._runProcess(process, params);
>     isStarted = await this._waitUntil(check);
>   } catch (e) {
>     // runProcess failed
>   }
>   
>   if (isStarted) {
>     onSuccessfulStart();
>   } else {
>     this.ready = false;
>     reject();
>   }

Both rewriting look awesome.  Thanks! I will address them.

(In reply to Julian Descottes [:jdescottes][:julian] from comment #115)
> Comment on attachment 8998714 [details]
> Bug 1469054 - Skip some test cases in test_adb.js on Windows.
> 
> https://reviewboard.mozilla.org/r/261640/#review269046
> 
> ::: devtools/shared/adb/test/test_adb.js:104
> (Diff revision 1)
> >  
> >    await extension.unload();
> >  });
> >  
> > -add_task(async function testNoTargetBinaries() {
> > +add_task({
> > +  skip_if: () => mozinfo.os == "win"
> 
> If you already have a bug logged to investigate those windows issues, can
> you mention it in a comment here?

Filed bug 1482008.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #117)
> (In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #73)
> > (In reply to Julian Descottes [:jdescottes][:julian] from comment #55)
> > > Comment on attachment 8993582 [details]
> > > Bug 1469054 - Make sure the adb server stops.
> > > 
> > > https://reviewboard.mozilla.org/r/258288/#review268722
> > > 
> > > Clearing the flag here, because I have a question about the stop() method,
> > > and this seems like a good spot to discuss it:
> > > 
> > > Some things I noticed while reviewing this:
> > > 1- we are only stopping ADB from tests now?
> > 
> > Yes, as of today.  I suppose we will do stop/start in the new
> > about:debugging.
> > 
> > > 2- we are no longer reacting to adb-start-polling and adb-stop-polling, is
> > > this intentional?
> > 
> > Right, it's intentional.  As commented above, I think devtools controls
> > stopping/starting adb.
> > 
> 
> Answering here before I forget: this new ADB will be used by both the new
> about:debugging and by WebIDE. The main priority is to get it to work with
> WebIDE in release 63. So we will need to start/stop ADB from WebIDE as well.
> This was the role of the LazyADB scanner I think. But we can address that
> when we reach step 3 of my comment 67 :)

Makes sense indeed.  Filed bug 1482010 for that.
Attachment #8998714 - Attachment is obsolete: true
Comment on attachment 8998713 [details]
Bug 1469054 - Make check() in adb-running-checker fail if a certain period of time elapsed.

https://reviewboard.mozilla.org/r/261638/#review269048

::: devtools/shared/adb/adb-running-checker.js:13
(Diff revision 1)
>   * Modified from adb-file-transfer from original ADB
>   */
>  
>  "use strict";
>  
> +const { setTimeout } = require("resource://gre/modules/Timer.jsm");

I was about to say that we are not importing clearTimeout but I just found out that the devtools loader was automatically injecting setTimeout and clearTimeout as "lazy globals":

https://searchfox.org/mozilla-central/rev/0f4d50ff5211e8dd85e119ef683d04b63062ed90/devtools/shared/builtin-modules.js#324-335

So we can remove the setTimeout imports from all the files in shared/adb/ since they are all loaded via require()
Attachment #8998713 - Flags: review?(jdescottes) → review+
Comment on attachment 8998723 [details]
Bug 1469054 - Skip some test cases in test_adb.js on Windows.

https://reviewboard.mozilla.org/r/261642/#review269050
Attachment #8998723 - Flags: review?(jdescottes) → review+
Attachment #8998723 - Attachment is obsolete: true
Comment on attachment 8998738 [details]
Bug 1469054 - Skip some test cases in test_adb.js on Windows.

https://reviewboard.mozilla.org/r/261644/#review269064

I thought I already r+'d this :/
Attachment #8998738 - Flags: review?(jdescottes) → review+
Comment on attachment 8998444 [details]
Bug 1469054 - Make sure the ADB server is ready to listen from clients when we call adb.start().

https://reviewboard.mozilla.org/r/261628/#review269066

Works for me, thanks!
Attachment #8998444 - Flags: review?(jdescottes) → review+
Julian, thanks for all of the reviews!

This will be a final try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d74af29bd2bef863047f93379f78e55f516c72ff

There is a lint failure but I've already fixed it.  And there is an orange on MacOSX, but it's not relevant with us at all! (I was actually shocked when I saw the orange.)
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b57faf0b3d1
Extract adb binary and related files into local profile directory from the extension. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/5bbec1fa2d07
Adapt implementations for adb.start() and adb.stop() and relevant stuff. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8e296137f5bf
Make sure the adb server stops. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/06690249322b
ESLint: Lint fixes. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ce992f3fd9b0
ESLint: Drop 'a' prefix from argument names. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/888d90ad5f93
ESLint: Drop function name. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/13135cde0d58
ESLint: Use integer for the radix on parseInt. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/b497d03cf428
ESLint: Add no-fallthrough annotation. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3151ffa250c2
Add async annotation for adb.start(). r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/bee6212d1727
Split Runtime.RuntimeTypes into a new file to import the RuntimeTypes in a file which is importing runtimes.js. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1de410c3efb3
Adapt scanner.js in the adbhelper addon into devtools. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/081d9ef214de
ESLint: lint fix for adb-scanner.js. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/7651e7eeb149
Adapt device.js in the adbhelper addon into devtools. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/41da6d248ce3
ESLint: lint fix for adb-device.js. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/610d28f29e34
Make sure that the adb.py sends all data. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/6a40296e9ca9
Make sure the ADB server is ready to listen from clients when we call adb.start(). r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/d241949106e8
Make check() in adb-running-checker fail if a certain period of time elapsed. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/e4db2e54dad6
Skip some test cases in test_adb.js on Windows. r=jdescottes
We should address the question about the "isUnpacked" logic that we had in the old adbhelper.
The new logic will unpack binaries every time it starts. Was this an intentional change?

If not, we should file a follow-up to implement it.
If yes, the platform manifest files from https://github.com/mozilla/devtools-adb-extension/ should be removed.
Flags: needinfo?(hikezoe)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #147)
> We should address the question about the "isUnpacked" logic that we had in
> the old adbhelper.
> The new logic will unpack binaries every time it starts. Was this an
> intentional change?
> 
> If not, we should file a follow-up to implement it.

I did put an item for that in bug 1481691. (The forth '*').
Flags: needinfo?(hikezoe)
Ah great that's perfect! 
I thought this was just about the "require" because the bug title mentioned "importing" ahah :)

Thank you for adding all my feedback in this new bug, I will be happy to help with it.
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f966891da20
edit type in devtools/client/webide/content/runtimedetails.js on a CLOSED TREE
Regressions: 1582020
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: