Closed Bug 585958 Opened 11 years ago Closed 2 years ago

webapps OS level integration : Maemo

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(fennec-)

RESOLVED WONTFIX
Tracking Status
fennec - ---

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(1 file, 9 obsolete files)

Each webapp should appear as an entry in the application list at the OS level.
Blocks: 583750
Attached patch WIP (obsolete) — Splinter Review
First pass at OS level support, with support for linux & maemo only :
- when adding or removing a web app, it is added to the OS application list
- the CLH recognizes --app=URI as a new parameter type, and opens a new window in this case.

For now, opening a web app when fennec is not already running doesn't work (Browser.startup() doesn't get called)
Assignee: nobody → fabrice
Attached patch WIP 2 (obsolete) — Splinter Review
This patch provides :
* install/uninstall launcher (Linux/Maemo)

* correct command line handling
  - webapps always open their own windows.
  - all other uris are grouped in the "real" fennec window.

* minimal UI for webapps
  - show the content and a stripped down title bar + favicon.
  - always hide the tab and tools bar.
  - update the window's title with the content title, to not get 10 "fennec"
    windows.
Attachment #465268 - Attachment is obsolete: true
Comment on attachment 465661 [details] [diff] [review]
WIP 2


>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>     this._setURI(caption);
>+    if (Browser.isApp)
>+      window.document.title = caption;

Why?

>+  enterAppMode: function() {
>+    this._edit.removeEventListener("click", this, false);
>+    this._edit.removeEventListener("mousedown", this, false);
>+    this._edit.disabled = true;
>+    let idBox = document.getElementById("identity-box");
>+    idBox.removeAttribute("onclick");
>+    idBox.removeAttribute("onkeypress");
>+    document.getElementById("urlbar-icons").style.display = "none";
>+  },

We'll probably need more than this. For example, we don't want the tabsidebar to be visible, I think.

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>+          // check for "app" flag
>+          var appFlag = cmdLine.handleFlagWithParam("app", false);

I think we should try to get this handler in the BrowserCLH, where we handle other commands. The CLH is also called while the app is running.

Also, let's use "webapp" as the flag. Mozilla already has an "app" flag.

>     // away panning for the total scroll offset.
>     doffset.add(panOffset);
>     Browser.tryFloatToolbar(doffset.x, 0);
>-    this._panScroller(Browser.controlsScrollboxScroller, doffset);
>+    if (!Browser.isApp)
>+      this._panScroller(Browser.controlsScrollboxScroller, doffset);

Why?

>diff --git a/components/BrowserCLH.js b/components/BrowserCLH.js

>-      win = Services.wm.getMostRecentWindow("navigator:browser");
>+    // we can only reuse the single "navigator:browser" window wich is not displaying a webapp

I wonder if we should change the windowtype to "navigator:webapp" of something. Need to check to see what Prism had to do (I forget)

>+    // web app support
>+    let app = aCmdLine.handleFlagWithParam("app", false);
>+    if (app) {
>+      let uri = resolveURIInternal(aCmdLine, app);
>+      if (uri) {
>+        win.browserDOMWindow.openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW, null);
>+        return;

OK, you have some support in the CLH, so I am wondering why you need any support in browser.js?

>diff --git a/modules/webapps.jsm b/modules/webapps.jsm
>         itemIds.forEach(function(aItem) {
>-		  if (bkmsvc.getFolderIdForItem(aItem) == self.rootId)
>+		  if (bkmsvc.getFolderIdForItem(aItem) == self.rootId) {
> 		    bkmsvc.removeItem(aItem);
>+        self._removeFromOS(aURI, aItem);
>+      }

You have some TABs in this patch
>+    let execd = Cc["@mozilla.org/file/directory_service;1"].  
>+                getService(Ci.nsIProperties).get("CurProcD", Ci.nsIFile);
>+    execd.append("fennec");
>+    let foStream = Cc["@mozilla.org/network/file-output-stream;1"].  
>+                    createInstance(Ci.nsIFileOutputStream);
>+    foStream.init(file, 0x02 | 0x08 | 0x20, 0666, 0); 
>+    let converter = Cc["@mozilla.org/intl/converter-output-stream;1"].
>+                    createInstance(Ci.nsIConverterOutputStream);

Feel free to not wrap these lines

>+      wbp.persistFlags &= ~Components.interfaces.nsIWebBrowserPersist.PERSIST_FLAGS_NO_CONVERSION;

Ci

>+    let file = Cc["@mozilla.org/file/directory_service;1"].  
>+                  getService(Ci.nsIProperties).get("Home", Ci.nsIFile);

No wrap


Just a quick first pass. I'll look deeper later (and at the other patches too)
(In reply to comment #3)
> Comment on attachment 465661 [details] [diff] [review]
> WIP 2
> 
> 
> >diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
> 
> >     this._setURI(caption);
> >+    if (Browser.isApp)
> >+      window.document.title = caption;
> 
> Why?

This sets the window title to the document's title. Without this, all the webapps windows (and the main fennec one) have the same title ("Fennec") and are hard to distinguish in the WM.

> >+  enterAppMode: function() {
> >+    this._edit.removeEventListener("click", this, false);
> >+    this._edit.removeEventListener("mousedown", this, false);
> >+    this._edit.disabled = true;
> >+    let idBox = document.getElementById("identity-box");
> >+    idBox.removeAttribute("onclick");
> >+    idBox.removeAttribute("onkeypress");
> >+    document.getElementById("urlbar-icons").style.display = "none";
> >+  },
> 
> We'll probably need more than this. For example, we don't want the tabsidebar
> to be visible, I think.

Sure, but it's taken care elsewhere

 
> >     // away panning for the total scroll offset.
> >     doffset.add(panOffset);
> >     Browser.tryFloatToolbar(doffset.x, 0);
> >-    this._panScroller(Browser.controlsScrollboxScroller, doffset);
> >+    if (!Browser.isApp)
> >+      this._panScroller(Browser.controlsScrollboxScroller, doffset);
> 
> Why?

This prevents the tabs and tools sidebars to appear.
 
> >diff --git a/components/BrowserCLH.js b/components/BrowserCLH.js
> 
> >-      win = Services.wm.getMostRecentWindow("navigator:browser");
> >+    // we can only reuse the single "navigator:browser" window wich is not displaying a webapp
> 
> I wonder if we should change the windowtype to "navigator:webapp" of something.
> Need to check to see what Prism had to do (I forget)
> 

I tried this, but it didn't work - this needs thorough investigation however since  it could simplify the command line handling.

> >+    // web app support
> >+    let app = aCmdLine.handleFlagWithParam("app", false);
> >+    if (app) {
> >+      let uri = resolveURIInternal(aCmdLine, app);
> >+      if (uri) {
> >+        win.browserDOMWindow.openURI(uri, null, Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW, null);
> >+        return;
> 
> OK, you have some support in the CLH, so I am wondering why you need any
> support in browser.js?

I found this needed to support all these scenarii: 
- start fennec, then open one or several web apps.
- start a web app, then open another one or the "classic" fennec.
- launch a web app from within fennec.

Thanks for the feedback!
OK, OK - Good answers, although we should work on pushing all commandline stuff to BrowserCLH and we should try to avoid Browser.isApp (having to check means it's easy for people to break the "app" mode without knowing)
Attached patch WIP 3 (obsolete) — Splinter Review
Addresses previous comments :
- no more Browser.isApp
- new windows have their own windowtype (navigator:webapp)

The chromeless mode is also improved, with deactivated commands when needed.

Current patch supports linux desktop & maemo. On maemo, webapps appear in the application list, but not in the shortcut list so can't be put on the desktop. This is tracked in https://bugs.maemo.org/show_bug.cgi?id=11163
Attachment #465661 - Attachment is obsolete: true
Summary: webapps OS level integration → webapps OS level integration : Maemo
Attached patch TAKE 2 - WIP 1 (obsolete) — Splinter Review
Web apps installation support for Maemo. We create a .deb file which is then installed by the application manager.

The deb is build by :
- creating all the needed files in a temporary directory.
- create the control.tar.gz & data.tar.gz files by calling /bin/tar
- finally create the .deb ourselves in JS since |dpkg-deb -b| is broken on Maemo (it expects GNU tar which is not what busybox provides)
Attachment #467598 - Attachment is obsolete: true
Attached patch TAKE 2 - WIP 2 (obsolete) — Splinter Review
This patch splits up the Debian packaging in its own module, and fixes a couple of bugs :
- be sure to generate a valid package name.
- fallback to default favicon in chrome if loading the image fails.

Note: this patch works also on ubuntu, with the limitation that the icon doesn't show up in the menus, since maemo and ubuntu look for icons in different directories.
Attachment #468098 - Attachment is obsolete: true
Attachment #468678 - Flags: review?(mark.finkle)
Comment on attachment 468678 [details] [diff] [review]
TAKE 2 - WIP 2


>diff --git a/modules/Makefile.in b/modules/Makefile.in

> EXTRA_JS_MODULES = \
>   linuxTypes.jsm \
>+  debian.jsm \

We should think about our naming convention for our modules. "linuxTypes.jsm" is kinda vague, considering what is in the file."debian.jsm" is vague too. "debianPackager.jsm" ?

>diff --git a/modules/debian.jsm b/modules/debian.jsm
>+  build: function(aBaseDir, aPackageDir, aPackageName, aControl) {
>+      }
>+    }
>+    // create control.tar.gz and data.tar.gz
>+    let controlTgz = this._createArchive(controlDir, aBaseDir, "control.tar.gz");

Use a blank line to separate some of the sections a little bit more. Just a little, here and there.

>+      let header = "!<arch>\n";
>+      // we need a buffered stream to use the writeFrom method
>+      let boStream = Cc["@mozilla.org/network/buffered-output-stream;1"].createInstance(Ci.nsIBufferedOutputStream);

Here too (above the comment)

>+      boStream.write(header, header.length);
>+      // hard code the debian-binary file entry, since tt's a simple text file
>+      this._appendPaddedString(boStream, "debian-binary", 16);

and here

>diff --git a/modules/webapps.jsm b/modules/webapps.jsm
>   _install: function(aURI, aTitle, aIconURI, aIconData) {

>+    let tmpDir = Cc["@mozilla.org/file/directory_service;1"].  
>+                  getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);

No wrap needed

>+    while (base64.length >= 64) {
>+      iconString += " " + base64.substring(0, 64) + "\n";
>+      base64 = base64.substring(64);
>+    }
>+    if (base64.length > 0)
>+      iconString += " " + base64 + "\n";

Blank line before "if"

>+    let aControl = {
>+      "Package": "moz-" + safeName,

You use the "moz-" prefix a few times, to create a unqiue name for the package, desktop and icon, I think - maybe more. I wonder if we should use something more like "webapp-"? I also wonder if these strings will be seen by the user during the install or uninstall?

>+    let iconListener = {
Add a short comment about why we need the listener, so other know why it's here

>+      QueryInterface: function(aIID) {
>+        if (aIID.equals(Ci.nsIWebProgressListener) ||
>+          aIID.equals(Components.interfaces.nsISupports))

No wrap

>+      onLocationChange: function() {return 0;},
>+      onProgressChange: function() {return 0;},
>+      onStatusChange: function() {return 0;},
>+      onSecurityChange: function() {return 0;},

Can we just remove the "return 0;" ?

>+    file.append("moz-" + safeName + "." + ext);
>+    let wbp = Cc['@mozilla.org/embedding/browser/nsWebBrowserPersist;1'].createInstance(Ci.nsIWebBrowserPersist);
>+    wbp.persistFlags = Ci.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES | Ci.nsIWebBrowserPersist.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION;
>+    wbp.progressListener = iconListener;
>+    let ios = Cc['@mozilla.org/network/io-service;1'].getService(Ci.nsIIOService);
>+    wbp.saveURI(ios.newURI(uri, "UTF8", null), null, null, null, null, file);

Add a blank line separator and a comment about what the webbrowserpersist is doing, actually saving the icon to a file

Great, clean patch! Just small nits, nothing major.
Attachment #468678 - Flags: review?(mark.finkle) → review+
Attached patch addressing comments (obsolete) — Splinter Review
I renamed the packager module to debianPackager.jsm, and addressed other comments.

About :

> You use the "moz-" prefix a few times, to create a unqiue name for the package,
> desktop and icon, I think - maybe more. I wonder if we should use something
> more like "webapp-"? I also wonder if these strings will be seen by the user
> during the install or uninstall?

Indeed the "moz-" prefix is used to create unique names for debian packages names, desktop files and icons. I'm not sure we should use the more generic "webapp-" or not : there is theoretically more chance of name conflicts with "webapp-". Chrome on linux uses a "chrome-" prefix, so using either we're safe with this.

The user never sees these ugly names. Both in the debian control file and in the desktop file we set an appropriate user-friendly display name, which is the application name that you can change when installing the web app.
Attachment #468678 - Attachment is obsolete: true
Attachment #472346 - Flags: review?(mark.finkle)
Attached patch addressing comments (obsolete) — Splinter Review
uploading correct version of the patch
Attachment #472346 - Attachment is obsolete: true
Attachment #472347 - Flags: review?(mark.finkle)
Attachment #472346 - Flags: review?(mark.finkle)
Comment on attachment 472347 [details] [diff] [review]
addressing comments

OK, thanks for the "moz-" explanation.
Attachment #472347 - Flags: review?(mark.finkle) → review+
Attached patch updated patch (obsolete) — Splinter Review
Patch updated to move to a component based approach. No change in the code.
Attachment #472347 - Attachment is obsolete: true
Attachment #473984 - Flags: review?(mark.finkle)
Attachment #473984 - Flags: review?(mark.finkle) → review+
Whiteboard: [fennec-checkin-postb1]
tracking-fennec: --- → ?
Whiteboard: [fennec-checkin-postb1]
tracking-fennec: ? → 2.0+
tracking-fennec: 2.0+ → 2.0b3+
Attached patch updated patch on current m-b (obsolete) — Splinter Review
Attachment #473984 - Attachment is obsolete: true
Attachment #488237 - Flags: review?(mark.finkle)
Comment on attachment 488237 [details] [diff] [review]
updated patch on current m-b

Let's not install any of the Maemo/Linux webapps components for Android. We should be able to just skip the components/webapps folder for Android, right?
Attachment #488237 - Flags: review?(mark.finkle) → review+
Prevents building components/webapps on android, and also remove linuxTypes.jsm and debianPackager.jsm from android.
Attachment #488237 - Attachment is obsolete: true
Attachment #488489 - Flags: review?(mark.finkle)
Moving to post fennec4
tracking-fennec: 2.0b3+ → 2.0-
Whiteboard: [sr:curtisk] → [secr:curtisk]
tickly to mfinkle are we ready to look at this one?
Mameo/MeeGo is not being developed by Mozilla. This patch and bug will be driven by the MeeGo team. It is unclear if they have any plans in turning this into submitted code.
Shall we just WONTFIX this unless someone wants to be an active owner here? I don't see why we'd want to waste time on this.
Romaxa works on the community version of Firefox for Mameo/Meego. Thoughts?
(In reply to Kevin Brosnan [:kbrosnan] from comment #21)
> Romaxa works on the community version of Firefox for Mameo/Meego. Thoughts?

Also, some non-UI parts (the .deb generator) could be reused for desktop linux on some distributions.
> Also, some non-UI parts (the .deb generator) could be reused for desktop
> linux on some distributions.
IIUC we should generate deb package on fly, and invoke system app handler, which would suggest to install package.
(In reply to Oleg Romashin (:romaxa) from comment #23)
> IIUC we should generate deb package on fly, and invoke system app handler,
> which would suggest to install package.

Yes, that's exactly what this patch provided for xul fennec on maemo.
so what prevents us to land it?
Whiteboard: [secr:curtisk] → [sec-assigned:curtisk:749334]
Attachment #488489 - Flags: review?(mark.finkle)
Flags: sec-review?(curtisk)
sec review is won't fix so removing the flag here
Flags: sec-review?(curtisk)
Whiteboard: [sec-assigned:curtisk:749334]
Closing all opened bug in a graveyard component
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.