Closed Bug 797075 Opened 8 years ago Closed 8 years ago

Implement Java-side tab stubs


(Firefox for Android :: General, defect)

Not set



Firefox 19


(Reporter: bnicholson, Assigned: bnicholson)




(2 files, 3 obsolete files)

There's a few different cases where we want to be able to create a new tab in Java (and have the UI updated accordingly) before a message is received from Gecko. We can accomplish this by stubbing tabs when we create them.
Attached patch Part 1: loadUrl() cleanup (obsolete) — Splinter Review
This patch cleans up the following:
- consolidates Tab:Add and Tab:Load into a single message (Tab:Load)
- factors out the new tab logic into a single function (needed for bug 797075)
- moves loadUrl() and loadUrlInTab() to to remove some references to mAppContext

One thing this patch is missing is setting the title of the tab immediately after loadUrl() is called, but this is fixed with the tab stubs in part 2.
Attachment #667205 - Flags: review?(mark.finkle)
Stubs tabs before receiving Tab:Added from Gecko. The easiest way to do this was to use negative IDs for stubbed tabs so that we could still use the mTabs id->tab mapping.
Attachment #667208 - Flags: review?(mark.finkle)
Comment on attachment 667205 [details] [diff] [review]
Part 1: loadUrl() cleanup

>+    public void loadUrl(String url, boolean newTab) {
>+        loadUrl(url, newTab, null, false, -1);
>+    }
>+    /**
>+     * Loads a tab with the given URL.
>+     *
>+     * @param url          URL of page to load, or search term used if searchEngine is given
>+     * @param newTab       if true, a new tab will be opened; otherwise, the selected tab is used
>+     * @param searchEngine if given, the search engine with this name is used
>+     *                     to search for the url string; if null, the URL is loaded directly
>+     * @param userEntered  whether the user manually typed the URL (used for disabling javascript: URIs)
>+     * @param parentId     ID of this tab's parent, or -1 if it has no parent
>+     */
>+    public void loadUrl(String url, boolean newTab, String searchEngine, boolean userEntered, int parentId) {

I like almost everything about this patch except the booleans used for newTab and userEntered, although newTab put me over the edge. Can we use integer flags for this?

Create a flags param to the methods and Tabs.LOADURL_NEW_TAB and Tabs.LOADURL_USER_ENTERED flag values? I based the names on the AWESOMEBAR values I saw.
Attachment #667205 - Flags: review?(mark.finkle) → review-
Changed to use flags.
Attachment #667205 - Attachment is obsolete: true
Attachment #667319 - Flags: review?(mark.finkle)
Comment on attachment 667319 [details] [diff] [review]
Part 1: loadUrl() cleanup, v2

Nice refactor
Attachment #667319 - Flags: review?(mark.finkle) → review+
This patch uses a different approach, which stores the tab counter in Java instead of Gecko. This allows Java to simply pass along the tab ID when creating a tab without any special stubbing logic. When creating a tab in Gecko, we can use JNI to pull the next ID.
Attachment #667208 - Attachment is obsolete: true
Attachment #667208 - Flags: review?(mark.finkle)
Attachment #667561 - Flags: review?(mark.finkle)
Attachment #667561 - Flags: review?(mark.finkle) → review+
This popped up in most of the failed tests on TBPL:

10-05 16:32:02.800 W/dalvikvm( 1712): JNI WARNING: illegal class name 'org.mozilla.gecko.Tabs' (Check_FindClass)
10-05 16:32:02.800 W/dalvikvm( 1712):              (should be formed like 'java/lang/String')

It looks like "/", not ".", should be used as the delimiter for findClass() strings. This change passes try:

I filed bug 799015 to fix our other findClass() call.
Attachment #667561 - Attachment is obsolete: true
Attachment #669024 - Flags: review?(mark.finkle)
Comment on attachment 669024 [details] [diff] [review]
Part 2: Implement Java-side tab stubs, v3

>+        let jni = new JNI();
>+        let cls = jni.findClass("org/mozilla/gecko/Tabs");
>+        let method = jni.getStaticMethodID(cls, "getNextTabId", "()I");
>+ = jni.callStaticIntMethod(cls, method);
>+        jni.close();

I wonder if it would be worth create a long-lived function for this, instead of creating the JNI stuff each time.

Could be a followup bug
Attachment #669024 - Flags: review?(mark.finkle) → review+
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Depends on: 799977
Depends on: 801675
Depends on: 800332
Depends on: 840823
Depends on: 878156
You need to log in before you can comment on or make changes to this bug.