Closed Bug 721004 Opened 12 years ago Closed 12 years ago

Load webapi+apps.js inside as a frame script instead of having to copy it to all applications

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(2 files, 2 obsolete files)

Patch v0.3
44.47 KB, patch
cjones
: review+
Details | Diff | Splinter Review
Revert nsTelephony.cpp changes
1.86 KB, patch
Details | Diff | Splinter Review
Attached patch Patch v0.2 (obsolete) — — Splinter Review 
Oups! the previous patch was having some parts that does not belong to this bug.
Attachment #591440 - Attachment is obsolete: true
Attachment #591440 - Flags: review?(jones.chris.g)
Attachment #591444 - Flags: review?(jones.chris.g)
Vivien,

Why do you need to transform the script in base64? Wouldn't messageManager.loadFrameSript(url, true) work?
Comment on attachment 591444 [details] [diff] [review]
Patch v0.2

Concern seconded.
Attachment #591444 - Flags: review?(jones.chris.g)
(In reply to Fabrice Desré [:fabrice] from comment #2)
> Vivien,
> 
> Why do you need to transform the script in base64? Wouldn't
> messageManager.loadFrameSript(url, true) work?

It doesn't. It seems like loading a frame script over http does not work.
Why don't we package the compat script in b2g chrome and load it through there?
> Why don't we package the compat script in b2g chrome and load it through there?

There is 2 reasons that come into my mind:
 - Others vendors probably don't want to use the code that live in webapi+apps.js
 - Is simpler and faster to land code for Gaia over Gaia. You do not have the 'm-i -> hg m-c -> git doublec -> git cgjones' train
(In reply to Vivien Nicolas (:vingtetun) from comment #6)
> > Why don't we package the compat script in b2g chrome and load it through there?
> 
> There is 2 reasons that come into my mind:
>  - Others vendors probably don't want to use the code that live in
> webapi+apps.js

Why not?  We're prototyping APIs to move into gecko.  Also, other vendors can ignore these shims if they want.

>  - Is simpler and faster to land code for Gaia over Gaia. You do not have
> the 'm-i -> hg m-c -> git doublec -> git cgjones' train

Yes, but what's happening here is promoting content JS to run with chrome privileges.  We need to review chrome-privileged code through appropriate m-c channels.  How frequently is webapi+apps.js changing?  The path from m-i --> b2g affects all gecko work, so if it's too slow, we should just fix that.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> (In reply to Vivien Nicolas (:vingtetun) from comment #6)
> > > Why don't we package the compat script in b2g chrome and load it through there?
> > 
> > There is 2 reasons that come into my mind:
> >  - Others vendors probably don't want to use the code that live in
> > webapi+apps.js
> 
> Why not?  We're prototyping APIs to move into gecko.  Also, other vendors
> can ignore these shims if they want.

I have no strong arguments against it but I though you were thinking the other way in https://bugzilla.mozilla.org/show_bug.cgi?id=712778#c3

> 
> >  - Is simpler and faster to land code for Gaia over Gaia. You do not have
> > the 'm-i -> hg m-c -> git doublec -> git cgjones' train
> 
> Yes, but what's happening here is promoting content JS to run with chrome
> privileges.  We need to review chrome-privileged code through appropriate
> m-c channels.  How frequently is webapi+apps.js changing?  The path from m-i
> --> b2g affects all gecko work, so if it's too slow, we should just fix that.

If will be hard to fix all m-i/m-c bustages ;)

I don't want to give chrome privileges to any part of the content - even the homescreen.
But his is a particular case in my opinion. This file should be removed asap. All the code in it should belong to a particular thing that should be fixed (mozApps/webApps, contacts, kinetic panning, settings, ...).

It could be also hard to rewrite all the UI code from scratch before MWC with slow trains...

Otherwise I'm 100% OK to add that to b2g/ so I will will fix that!
Attached patch Patch v0.3 — — Splinter Review 
This version move webapps+api.js to b2g/
It has been renamed webapi.js because our build system does not like '+' in a filename.

Also I have let a lot of var because I plan to re-used this file with a few modifications to keep alive the web demo. (This is one of the main tool used by the UI/UX team).
Attachment #591444 - Attachment is obsolete: true
Attachment #591773 - Flags: review?(jones.chris.g)
(In reply to Vivien Nicolas (:vingtetun) from comment #8)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> > (In reply to Vivien Nicolas (:vingtetun) from comment #6)
> > > > Why don't we package the compat script in b2g chrome and load it through there?
> > > 
> > > There is 2 reasons that come into my mind:
> > >  - Others vendors probably don't want to use the code that live in
> > > webapi+apps.js
> > 
> > Why not?  We're prototyping APIs to move into gecko.  Also, other vendors
> > can ignore these shims if they want.
> 
> I have no strong arguments against it but I though you were thinking the
> other way in https://bugzilla.mozilla.org/show_bug.cgi?id=712778#c3
> 

My objection there was just that it was so far away from the proposed settings API that it wouldn't have helped us move forward on the "real" impl.  It was more of a private DB shared between the settings app and the homescreen.

> > 
> > >  - Is simpler and faster to land code for Gaia over Gaia. You do not have
> > > the 'm-i -> hg m-c -> git doublec -> git cgjones' train
> > 
> > Yes, but what's happening here is promoting content JS to run with chrome
> > privileges.  We need to review chrome-privileged code through appropriate
> > m-c channels.  How frequently is webapi+apps.js changing?  The path from m-i
> > --> b2g affects all gecko work, so if it's too slow, we should just fix that.
> 
> If will be hard to fix all m-i/m-c bustages ;)
> 
> I don't want to give chrome privileges to any part of the content - even the
> homescreen.
> But his is a particular case in my opinion. This file should be removed
> asap. All the code in it should belong to a particular thing that should be
> fixed (mozApps/webApps, contacts, kinetic panning, settings, ...).
> 

Agreed!

> It could be also hard to rewrite all the UI code from scratch before MWC
> with slow trains...
> 

Why would we need to do that?
Comment on attachment 591773 [details] [diff] [review]
Patch v0.3

>diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js

This is unrelated, let's do it in a separate bug.

>diff --git a/b2g/chrome/content/shell.js b/b2g/chrome/content/shell.js
>+    // Load webapi+apps.js as a frame script
>+    try {
>+      let url = 'chrome://browser/content/webapi.js';
>+      messageManager.loadFrameScript(url, true);
>+    } catch (e) {
>+      dump('Error when loading ' + url + ' as a frame script: ' + e + '\n');

|url| is scoped to the try { } block, out of scope in catch() { }.
 Need to declare it in the outer scope.

r=me with scoping fix and removal of unrelated pref changes.
Attachment #591773 - Flags: review?(jones.chris.g) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> Comment on attachment 591773 [details] [diff] [review]
> Patch v0.3
> 
> >diff --git a/b2g/app/b2g.js b/b2g/app/b2g.js
> 
> This is unrelated, let's do it in a separate bug.
> 

Sorry this is a leftover from https://bugzilla.mozilla.org/show_bug.cgi?id=720831
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdd9fc77c468

And there is also some changes in nsTelephony that I have pushed by error... patch to revert those changes coming...
Please back out the entire rev and reland with just the right changes applied.
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/2aab3e84a1ee

(For clarity when marking bugs post merge, please can you start backout commit messages with "Backout <cset> <and optionally (bug #)> reason" :-) Mak's backout script does this pretty nicely/easily https://wiki.mozilla.org/User:Mak77)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: