Closed Bug 1168562 Opened 5 years ago Closed 4 years ago

B2G Installer Addon

Categories

(Firefox OS Graveyard :: B2gInstaller, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S14 (12june)

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 1 obsolete file)

Attached file Github repo
Bug for tracking implmentation of the addon itself
Depends on: 1164290
Depends on: 1059081
Whiteboard: [systemsfe]
And I pushed mac binaries and I could flash a Z3 from a Mac. This is getting cool.
(In reply to Alexandre LISSY :gerard-majax from comment #1)
> And I pushed mac binaries and I could flash a Z3 from a Mac. This is getting
> cool.

Rebuilt the binaries as 64 bits and static. No idea why those were failing yesterday. I could flash my Z3 a dozen of times from a mac \o/
Comment on attachment 8610785 [details]
Github repo

Fabrice, let's formally consider the current code for reviewing. The goal is to assert a first version upon which Etienne and others will be able to build on to provide nice UX.
Attachment #8610785 - Flags: review?(fabrice)
Comment on attachment 8610785 [details]
Github repo

In about.js, please get rid of all the |let deferred = promise.defer();| and use the Promise object provided by the platform. I think I did it for all the functions in https://github.com/fabricedesre/b2g-installer/blob/master/addon/about.js
It would be nice to document what the promises resolve to because some functions are a bit hard to follow. For instance, dealWithBlobFree() needs some refactoring...

Also, all the DOM stuff needs to go away from this file, but I guess we can let that up to Etienne.
Attachment #8610785 - Flags: review?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #4)
> Comment on attachment 8610785 [details]
> Github repo
> 
> In about.js, please get rid of all the |let deferred = promise.defer();| and
> use the Promise object provided by the platform. I think I did it for all
> the functions in
> https://github.com/fabricedesre/b2g-installer/blob/master/addon/about.js
> It would be nice to document what the promises resolve to because some
> functions are a bit hard to follow. For instance, dealWithBlobFree() needs
> some refactoring...

Done for both. I agree the code was not very very easy to follow. That should be better now.

> 
> Also, all the DOM stuff needs to go away from this file, but I guess we can
> let that up to Etienne.

So about.js should only be dealing with UI and we would sendAsyncMessage() with main process for all the real logic?
Target Milestone: --- → 2.2 S14 (12june)
Comment on attachment 8610785 [details]
Github repo

That should be better looking. I have reworked all the promises to make use of only platform Promise object.

I have also reworked the nasty dealWithBlobFree() to make it easier to read, track, and hack.

I also added a small option for the flashing that allows one to keep the userdata partition untouched if:
 - he wants
 - the device already runs B2G

That way people will be able to reflash a new B2G base image while keeping their data.

Finally, I've also identified what may have been your issue while testing when you got images that were not booting past the SONY logo. Turns out that when we |adb root| adb gets disconnected and comes back, and that may have messed up with blobs retrieval. At least, I did reproduce your issue twice (over 20 flashes maybe, but not all from scratch) and each time the symptoms were the same:
 - not passing the SONY logo
 - rebooting back
 - in the temp directory's the boot.img and recovery.img were smaller than expected
 - in the temp directory's storing blobs locally, most of the one we pull at first were missing

I've added a not so nice setTimeout() of 5 secs to get rid of this. So far, not reproduced.

All those changes are done on top of the first commit you reviewed to ease tracking.
Attachment #8610785 - Flags: review?(fabrice)
(In reply to Alexandre LISSY :gerard-majax from comment #5)

> So about.js should only be dealing with UI and we would sendAsyncMessage()
> with main process for all the real logic?

No, all the logic could move in some backend.js and the front end in about.js/frontend.js. Just see that as a library, but don't use the message manager as an interface.
Bug 1026945 will impact us as it will make stop ADBHelper to be always-on. We will need to trigger it ourselves.
Depends on: 1026945
Can you upload it as a patch or do a PR? I can't let comments anywhere currently.
So it's just the same code but all squashed into one commit, and attached on
Bugzilla, that should make it easier to review :)
Attachment #8615531 - Flags: review?(fabrice)
Attachment #8610785 - Flags: review?(fabrice)
Comment on attachment 8615531 [details] [diff] [review]
Provide an addon to insall B2G onto a supported device

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

Apart from the nits, what I'd really like to see improved in about.js : 
- document the method parameters.
- maybe encapsulate all these global methods... I feel that we'll refactor anyway when doing the real front-end.

::: Makefile
@@ +1,5 @@
> +FILES=about.css about.js about.xhtml bootstrap.js imaging_tools.js main.js subprocess.js chrome.manifest
> +ADDON_VERSION=0.1
> +XPI_NAME=b2g-installer-$(ADDON_VERSION)
> +
> +XPIS = $(XPI_NAME)-win32.xpi $(XPI_NAME)-linux.xpi $(XPI_NAME)-linux64.xpi $(XPI_NAME)-mac64.xpi

I think we should rather do a single xpi instead of one per platform. Also, do we really support windows?

::: about.js
@@ +33,5 @@
> +const kExpectedBlobFreeContent = [
> +  kBlobFree, kBlobsInject, kCmdlineFs, kDevicesJson, kDeviceRecovery
> +];
> +
> +const kB2GInstallerTmp = "/tmp/b2g-installer/";

don't hardcode that, use https://github.com/fabricedesre/b2g-installer/blob/master/addon/about.js#L41

@@ +110,5 @@
> +        let _src = src[0] === "/" ? src.slice(1) : src;
> +        let f = new FileUtils.File(OS.Path.join(blobsDir.path, _src));
> +        currentBlob++;
> +
> +        device.pull(src, f.path).then((res) => {

here and everywhere else: then(res => {... when there is a single argument.

@@ +928,5 @@
> +
> +addEventListener("load", function load() {
> +  removeEventListener("load", load, false);
> +
> +  // getAllDevices();

nit: remove

::: bootstrap.js
@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const REASON = [ 'unknown', 'startup', 'shutdown', 'enable', 'disable',
> +                 'install', 'uninstall', 'upgrade', 'downgrade' ];

Be consistent with double quotes for strings.

@@ +43,5 @@
> +
> +  let uri = registerAddonResourceHandler(data);
> +
> +  let loaderModule =
> +    Cu.import('resource://gre/modules/commonjs/toolkit/loader.js').Loader;

here too

::: main.js
@@ +39,5 @@
> +// Proper execution of the full process depends on some bugs fixed:
> +// - Bug 1059081: TCPSocket crash in nsMultiplexInputStream::ReadSegments on
> +//                "Socket Thread" due to apparent racey use of
> +//                nsMultiplexInputStream
> +// - Bug 1164290: ZipUtils.extractFiles() breaks with symbolic links

they are both fixed, no need for this comment anymore

::: template-install.rdf
@@ +18,5 @@
> +    <!-- Firefox -->
> +    <em:targetApplication>
> +      <Description>
> +        <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> +        <em:minVersion>33.0</em:minVersion>

I don't think that's the right minVersion
Attachment #8615531 - Flags: review?(fabrice) → review+
This should address all the comments
Attachment #8615531 - Attachment is obsolete: true
Attachment #8616664 - Flags: review?(fabrice)
Attachment #8616664 - Flags: review?(fabrice) → review+
Rebased everything into one first commit: https://github.com/lissyx/b2g-installer/commit/dd3b28f5733ee65607fcdc01501632ada0991653
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1175176
Component: General → B2gInstaller
You need to log in before you can comment on or make changes to this bug.