Last Comment Bug 701527 - Add support for IndexedDB
: Add support for IndexedDB
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Linux
P2 normal (vote)
: ---
Assigned To: Matt Brubeck (:mbrubeck)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on: 705012
Blocks: 702796
  Show dependency treegraph
 
Reported: 2011-11-10 14:21 PST by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2012-02-21 13:40 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
part 1: transplant code from /browser (5.88 KB, patch)
2011-11-11 11:57 PST, Matt Brubeck (:mbrubeck)
mark.finkle: review+
Details | Diff | Splinter Review
part 2: (WIP) (8.19 KB, patch)
2011-11-11 11:58 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
part 2: use NativeWindow.doorhanger and BrowserApp APIs (8.14 KB, patch)
2011-11-14 15:59 PST, Matt Brubeck (:mbrubeck)
mark.finkle: review+
Details | Diff | Splinter Review
part 3: Hide the doorhanger on cancel (8.28 KB, patch)
2011-11-15 12:07 PST, Matt Brubeck (:mbrubeck)
margaret.leibovic: review-
Details | Diff | Splinter Review
part 3 (v2): Hide the doorhanger on cancel (6.38 KB, patch)
2011-11-15 13:40 PST, Matt Brubeck (:mbrubeck)
margaret.leibovic: review+
Details | Diff | Splinter Review

Description User image Mark Finkle (:mfinkle) (use needinfo?) 2011-11-10 14:21:21 PST
We can mostly port the desktop Firefox helper object for this:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6475

Except we use NativeWindow.doorhanger

Since this is kinda async, like e10s Fennec, you might want to look at the XUL Fennec impl too:
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/browser.js#723

It might help make things a little cleaner. Also, use the BrowserApp.getXxx helpers instead of those built into the desktop OfflineApps
Comment 1 User image Matt Brubeck (:mbrubeck) 2011-11-11 11:57:51 PST
Created attachment 573876 [details] [diff] [review]
part 1: transplant code from /browser

Just copies and pastes code from desktop's browser.js, with a find-and-replace to change "var" to "let".  Just separating this out to make reviews easier, so you can see which code is old and which is now.  I'll fold this into the other patch(es) before landing, since it doesn't work on its own.
Comment 2 User image Matt Brubeck (:mbrubeck) 2011-11-11 11:58:45 PST
Created attachment 573879 [details] [diff] [review]
part 2: (WIP)

Converts the code to use native doorhangers and BrowserApp methods.  This is more or less complete but it isn't working yet.  I'm debugging it now.
Comment 3 User image Matt Brubeck (:mbrubeck) 2011-11-14 15:59:11 PST
Created attachment 574461 [details] [diff] [review]
part 2: use NativeWindow.doorhanger and BrowserApp APIs

It turns out my tests were failing because my test case was using obsolete APIs.  This patch actually works fine.

I'll probably post a follow-up to make this use the doorhanger timeout/persistence features that just landed in bug 700913.
Comment 4 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-11-14 16:48:16 PST
Comment on attachment 574461 [details] [diff] [review]
part 2: use NativeWindow.doorhanger and BrowserApp APIs

We might need a new doorhanger.hide(...) API
Comment 5 User image Matt Brubeck (:mbrubeck) 2011-11-15 12:07:32 PST
Created attachment 574645 [details] [diff] [review]
part 3: Hide the doorhanger on cancel

This adds a doorhanger.hide method, so the quota-cancel method can do the same thing as XUL Fennec or desktop Firefox.  It adds a Tab.forceRemoveDoorHanger method that ignores persistence and timeout values, and also uses that to avoid creating duplicate doorhangers in addDoorHanger.
Comment 6 User image :Margaret Leibovic 2011-11-15 12:23:56 PST
Comment on attachment 574645 [details] [diff] [review]
part 3: Hide the doorhanger on cancel

I'm planning on landing my patch for bug 702386 today, and it fixes persistence/timeout checks so that you won't need to make a forceRemoveDoorHanger method.

I'm sorry it didn't land before you wrote this, but hopefully it will make this simpler! (That patch also fixes the issue I see you found in addDoorHanger.)
Comment 7 User image Matt Brubeck (:mbrubeck) 2011-11-15 13:40:30 PST
Created attachment 574676 [details] [diff] [review]
part 3 (v2): Hide the doorhanger on cancel

Now with LESS CODES.
Comment 8 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-11-15 13:45:30 PST
Comment on attachment 574676 [details] [diff] [review]
part 3 (v2): Hide the doorhanger on cancel

The indexexDB prompts are currently unique in that they really timeout, so we do need to allow them to be removed from the Java side. This timeout is not the same as the DoorHanger persistence/timeout.
Comment 9 User image :Margaret Leibovic 2011-11-15 14:35:08 PST
Comment on attachment 574676 [details] [diff] [review]
part 3 (v2): Hide the doorhanger on cancel

>diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java
>--- a/embedding/android/GeckoApp.java
>+++ b/embedding/android/GeckoApp.java

>+        mMainHandler.post(new Runnable() {
>+            public void run() {
>+                Tab tab = Tabs.getInstance().getTab(tabId);
>+                if (tab == null)
>+                    return;
>+                tab.removeDoorHanger(value);

This just removes the doorhanger from the set of doorhangers stored on the tab, so you'll also need a mDoorHangerPopup.updatePopupForTab(tab) here if you want to make sure the notification disappears from the UI (or mDoorHangerPopup.updatePopup() if bug 702778 lands).
Comment 10 User image Matt Brubeck (:mbrubeck) 2011-11-15 15:02:31 PST
(In reply to Margaret Leibovic [:margaret] from comment #9)
> This just removes the doorhanger from the set of doorhangers stored on the
> tab, so you'll also need a mDoorHangerPopup.updatePopupForTab(tab) here if
> you want to make sure the notification disappears from the UI (or
> mDoorHangerPopup.updatePopup() if bug 702778 lands).

Thanks.  I added updatePopupForTab, and I see you have already landed bug 702778 and changed it to updatePopup:

https://hg.mozilla.org/projects/birch/rev/5675736cb251 (parts 1 and 2)
https://hg.mozilla.org/projects/birch/rev/144775136472 (part 3)

I also filed follow-up bug 702796 to add the 5-minute timeout behavior from desktop Firefox.

No tests; do we have a good way to run unit tests for this code yet?
Comment 11 User image :Margaret Leibovic 2011-11-15 15:18:17 PST
(In reply to Matt Brubeck (:mbrubeck) from comment #10)
> No tests; do we have a good way to run unit tests for this code yet?

I was asking around about that last week. I think once bug 701076 lands we will have a framework in place to let us test native UI components.
Comment 12 User image Gian-Carlo Pascutto [:gcp] 2011-11-16 00:40:00 PST
mobile/components/ContentPermissionPrompt.js also claims to handle IndexedDB requests. I gather from this bug that it actually doesn't. Maybe we should clean it up to prevent future confusion?

http://hg.mozilla.org/projects/birch/file/5352c48874e2/mobile/components/ContentPermissionPrompt.js#l76

Note You need to log in before you can comment on or make changes to this bug.