554271 - [e10s] Fennec Bookmarks UI does not work with e10s fennec

Status: VERIFIED DUPLICATE of bug 560827

(Firefox for Android Graveyard :: Bookmarks, defect)

Description

[Oleg Romashin (:romaxa)]

Start e10s fennec.
Try to add some page to bookmark with "Star" button

EXP: bookmark added, dialog "Edit/Remove" appeared
ACT: nothing happen.

Comment 1

[chithraprabha]

Currently I am looking into this bug,Looks like there is no implementation to get the CurrentURI with IPC in the Chrome Process.

Comment 2

[chithraprabha]

Can I use getBrowser().getAttribute("src") in place of browser.currentURI? 

But looks like "getBrowser().getAttribute("src")"  returns a valid uri value, but with some type conversion error below,

JavaScript error: , line 0: uncaught exception: [Exception... "Could not convert JavaScript argument arg 0 [nsINavBookmarksService.getBookmarkIdsForURI]"  nsresult: "0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame :: file:///home/chithrp/E10s/mobilebase/mobile/dist/bin/xulrunner/modules/utils.js :: PU_getMostRecentBookmarkForURI :: line 994"  data: no]

please share  any  suggestions about the  correct way to get the currentURI value and to pass it to the bookmark services APIs.

Comment 3

[Doug Turner (:dougt)]

ben, ^

Comment 4

[chithraprabha]

Attachment: Patch V.0

Added a new API "getCurrentURI()" to get the CurrentURI, this replaces 'browser.currentURI',fixes the JS exceptions created by the same.

With this patch Bookmarks UI is displayed ok, Edit and Remove bookmarks functionality works fine.

Comment 5

[Frederic Plourde (:fred23)]

Comment on attachment 438050 [details] [diff] [review]
Patch V.0

>+function getCurrentURI() {
>+  var tab = Browser.selectedTab;
>+  var browser = tab.browser;
>+  var uri =  tab.getURI();
>+  return gURIFixup.createExposableURI(uri);
>+}

What do you need 'var browser' for ?  
It seems unnecessary here and you could just do :

function getCurrentURI() {
  var uri =  Browser.selectedTab.getURI();
  return gURIFixup.createExposableURI(uri);
}

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't want to sound too negative, but browser.mTitle and getCurrentURI() are not quite what I have in mind.

browser.contentTitle already exists and we should reuse it for e10s. In fact, we should make the <browser> object able to handle e10s APIs and just re-implement the browser properties.

Comment 7

[chithraprabha]

Ok,I can look into re-implementing the browser properties.

Also one more function related to BookmarkUI  'updateStar',requires the similar for  currentURI,Currently this function is commented out with no proper comments,the patch  below enables updating the status of the star button.

diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js
--- a/chrome/content/browser-ui.js
+++ b/chrome/content/browser-ui.js
@@ -485,19 +485,17 @@ var BrowserUI = {
     // Make sure we're online before attempting to load
     Util.forceOnline();
 
     var submission = button.engine.getSubmission(this._edit.value, null);
     Browser.messagePasser.sendMessage(getBrowser(), "FennecSearch", submission.uri.spec, submission.postData);
   },
 
   updateStar : function() {
-    // XXX
-    return;
-    if (PlacesUtils.getMostRecentBookmarkForURI(Browser.selectedBrowser.currentURI) != -1)
+    if (PlacesUtils.getMostRecentBookmarkForURI(getCurrentURI()) != -1)
       this.starButton.setAttribute("starred", "true");
     else
       this.starButton.removeAttribute("starred");
   },
 
   newTab : function newTab(aURI) {
     aURI = aURI || "about:blank";
     let tab = Browser.addTab(aURI, true);

mfinkle,Can I add this fix also in my next patch?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> Ok,I can look into re-implementing the browser properties.
> 
> Also one more function related to BookmarkUI  'updateStar',requires the similar
> for  currentURI,Currently this function is commented out with no proper
> comments,the patch  below enables updating the status of the star button.

> mfinkle,Can I add this fix also in my next patch?

Yes

Comment 9

[chithraprabha]

(In reply to comment #6)
> I don't want to sound too negative, but browser.mTitle and getCurrentURI() are
> not quite what I have in mind.
> 
> browser.contentTitle already exists and we should reuse it for e10s. In fact,
> we should make the <browser> object able to handle e10s APIs and just
> re-implement the browser properties.

About reusing this browser.contentTitle , it is again a property defined in bindings for browser xul and it doesn't work if  remote="true".

And if I have a look at the bindings in browser.xml, I guess the getting nsIWebNavigation object is currently not possible in this file ,for implementing any methods or properties.

Also my understanding is the methods in the bindings are  some how replaced using the "messagePasser" ?? eg:- http://hg.mozilla.org/users/pavlov_mozilla.com/mobile-e10s/rev/1ebb3f19fdf5 .

So for re-implementing the properties, we might need some alternative to get the nsIWebNavigation interface, please suggest some ways I can do this ? 

I could think of 
1.Using the 'messagePasser' , this would require us to replace the properties's names every where and no a good idea?
2.If we need to use the existing properties with same name, we have to have some API (via IPDL ? ) which will allow us to get the nsIWebNavigation interface in brower.xml bindings ,when we have remote='true'.

I have tried to replace the currentURI property ,some thing like below 

      <property name="currentURI"
                onget="return Components.classes['@mozilla.org/docshell/urifixup;1'].getService(Components.interfaces.nsIURIFixup).createExposableURI(this.getTabBrowser().getURI());"
                readonly="true"/>

I got this error,
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /home/chithrp/E10-Apr-15/electrolysis/netwerk/base/src/nsIOService.cpp, line 583
JavaScript error: , line 0: uncaught exception: [Exception... "'[JavaScript Error: "this.getTabBrowser() is null" {file: "chrome://global/content/bindings/browser.xml" line: 0}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/commandUtil.js :: anonymous :: line 140"  data: yes]

so Please suggest your idea for re-implementing the browser properties,might be I am missing some good ways..

Comment 10

[Benjamin Stover (:stechz)]

Hey chithraprabha,

The way we currently do browser navigation / getting the title / getting the URI is a little hacky in electrolysis.  Instead of using browser-binded methods which don't work when remote=true, we assign our own custom stuff to the browser element in a few places.  See here for instance:

http://hg.mozilla.org/users/pavlov_mozilla.com/mobile-e10s/file/b798302abc67/chrome/content/browser.js#l2506

This object sends and receives messages from frame-content.js and sets properties on browser (fennecCanGoBack/Forward, URL, title).  One idea is to insulate all of this into our own fennec-specific browser bindings.  This is a much larger refactoring project that should be a different bug.

For this bug your use of tab.getURI() is fine.  I don't think this merits a new global function.

By the way, thanks for helping us out :)

Comment 11

[chithraprabha]

Attachment: Updated Patch V.1

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

I have a patch to the <browser> object that obsoletes this patch.

Comment 13

[chithraprabha]

Attachment: Seperate properties in bindings ,when 'remote = true'

Comment 14

[chithraprabha]

(In reply to comment #12)
> I have a patch to the <browser> object that obsoletes this patch.

Ok,mfinlke,could you please add here the dependency on the bug in which this patch would available, I can fix this bug when  the patch is available.

It would be nice to have the fixes in XBL , I just tried some samples ,I have attached in  https://bugzilla.mozilla.org/attachment.cgi?id=440490 , but couldn't get the correct way to get the browser object's URI that could be used with bookmarks API. this sample worked ok to get a URI from browser object , but this wasn't enough to fix the bookmark UI.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

The patch I have in bug 560827 fixes this bug

Comment 16

[Doug Turner (:dougt)]

mfinkle, dup against your bug?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

OK. Also, I don't think we want to over-use CPOWs, which is another reason I'd like to go with the approach in bug 560827.