Closed Bug 566640 Opened 14 years ago Closed 14 years ago

[f10s] Move mIconURL to browser.xml

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android 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, 5 obsolete files)

Patch
16.90 KB, patch
Details | Diff | Splinter Review
Patch v0.4
4.69 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
Attached patch Patch (obsolete) — — Splinter Review 
The attached patch move mIconURL to browser.xml and also enable the opensearch engines.

This is my first real code refactoring directly into e10s dealing with chrome and web content, so feel free to add a bunch of commentary :)
Attachment #445986 - Flags: review?(webapps)
Attachment #445986 - Flags: review?(mark.finkle)
Comment on attachment 445986 [details] [diff] [review]
Patch

>diff -r 30c0f926f2e2 chrome/content/bindings/browser.js

>+      case "DOMLinkAdded":
>+        let link = aEvent.originalTarget;
>+        let ownerDoc = link.ownerDocument;
>+        // Only check links that are in the root document.
>+        if (!link || !link.href || !ownerDoc || ownerDoc.defaultView.frameElement)
>+          return;

I wonder if we should skip the "ownerDoc.defaultView.frameElement" check and move it into the returned JSON. Since we are sending the DOMNLinkAdded as a message, I think we should send the message for any document. Then the receiver can filter.

>+        let type = "";
>+        if (/\bicon\b/i(link.rel)) {
>+          type = "icon";
>+        }
>+        else if (/\bsearch\b/i(link.rel) && link.type && link.title) {
>+          let linkType = link.type.replace(/^\s+|\s*(?:;.*)?$/g, "").toLowerCase();
>+          if (linkType == "application/opensearchdescription+xml" && /^(?:https?|ftp):/i.test(link.href)) {
>+            type = "search";
>+          }
>+        }

Do we really want to do this work in the frame-script? Maybe we should just let the browser.xml (or other receiver) handle it?

>+        let json = {
>+          type: type,
>+          title: link.title,
>+          href: link.href,
>+          characterSet: content.document.characterSet
>+        };

If we move the code to parse the link into the chrome side, let's change "type: type," to "type: link.type," and we should include any other attributes that appear on the link.

>diff -r 30c0f926f2e2 chrome/content/bindings/browser.xml

>+      <field name="searchEngines">[]</field>

I wonder if this should be a readonly property (based on a _searchEngines field)

>       <method name="receiveMessage">
>         <parameter name="aMessage"/>
>-        <body>
>-        <![CDATA[
>-         switch (aMessage.name) {
>+        <body><![CDATA[
>+          switch (aMessage.name) {
>             case "DOMTitleChanged":
>             {
>-               this._contentTitle = aMessage.json.title;
>+              this._contentTitle = aMessage.json.title;
> 
>-               let event = document.createEvent("Events");
>-               event.initEvent("DOMTitleChanged", true, true);
>-               this.dispatchEvent(event);
>-               break;
>+              let event = document.createEvent("Events");
>+              event.initEvent("DOMTitleChanged", true, true);
>+              this.dispatchEvent(event);
>+              break;
>             }
>             case "DOMWillOpenModalDialog":
>             {
>-               let event = document.createEvent("Events");
>-               event.initEvent("DOMWillOpenModalDialog", true, true);
>-               this.dispatchEvent(event);
>-               break;
>+              let event = document.createEvent("Events");
>+              event.initEvent("DOMWillOpenModalDialog", true, true);
>+              this.dispatchEvent(event);
>+              break;
>             }
>             case "DOMWindowClose":
>             {
>-               let event = document.createEvent("Events");
>-               event.initEvent("DOMWindowClose", true, true);
>-               this.dispatchEvent(event);
>-               break;
>+              let event = document.createEvent("Events");
>+              event.initEvent("DOMWindowClose", true, true);
>+              this.dispatchEvent(event);
>+              break;
>+            }
>+            case "DOMLinkAdded":
>+            {
>+              let link = aMessage.json;
>+              switch(link.type) {
>+                case "icon":
>+                {
>+                  let iconURI = gIOService.newURI(link.href, link.characterSet, null);
>+                  if (!iconURI.schemeIs("javascript") && !gFaviconService.isFailedFavicon(iconURI)) {
>+                    gFaviconService.setAndLoadFaviconForPage(this.currentURI, iconURI, true);
>+                    this.mIconURL = iconURI.spec;
>+                  }
>+
>+                  let event = document.createEvent("Events");
>+                  event.initEvent("DOMFaviconChanged", true, true);
>+                  this.dispatchEvent(event);

Let's _not_ fire this event for two reasons: 1. DOMFaviconChange is not a real event and we don't want to make new events if we can help it, 2. We don't want to re-dispatch _any_ events (DOMTileChanged and others are going away too) and we want people to receive the messages themselves, not fake events.

>+                case "search":
>+                {
>+                  this.searchEngines.push({ title: link.title, href: link.href });
>+
>+                  let event = document.createEvent("Events");
>+                  event.initEvent("DOMSearchEngineAdded", true, true);
>+                  this.dispatchEvent(event);
>+                  break;

Same here. People should listen for the "DOMLinkAdded" message, not a new fake event.

>diff -r 30c0f926f2e2 chrome/content/browser-ui.js

>     // XXX these really want to listen to only the current browser
>     browsers.addEventListener("DOMTitleChanged", this, true);
>+    browsers.addEventListener("DOMFaviconChanged", this, true);
>     browsers.addEventListener("DOMWillOpenModalDialog", this, true);

Add this to the window.messageManager.addMessageListener("DOMLinkAdded", ...);, since we don't want to use fake events.

>+      case "DOMFaviconChanged":
>+        this._updateIcon(browser.mIconURL);
>+        break;

Move to receiveMessage handler and use "DOMLinkAdded"

>diff -r 30c0f926f2e2 chrome/content/browser.js
Attachment #445986 - Flags: review?(mark.finkle) → review-
Comment on attachment 445986 [details] [diff] [review]
Patch

>diff -r 30c0f926f2e2 chrome/content/browser-ui.js

>       case TOOLBARSTATE_LOADING:
>         if (icons.getAttribute("mode") != "edit")
>           this._updateToolbar();
> 
>-        browser.mIconURL = "";
>+        browser.mIconURL = browser.currentURI.prePath + "/favicon.ico";

We can't do this here. Bug 515188 and the regression, bug 523369, tell us that we should use "browser.contentDocument.documentURIObject.prePath", but we can't in e10s. Therefore, we should leave it alone.

In order to set the default favicon, we may need to send back the documentURIObject.prePath or .spec in one of the other messages.
(In reply to comment #3)
> (From update of attachment 445986 [details] [diff] [review])
> >diff -r 30c0f926f2e2 chrome/content/browser-ui.js
> 
> >       case TOOLBARSTATE_LOADING:
> >         if (icons.getAttribute("mode") != "edit")
> >           this._updateToolbar();
> > 
> >-        browser.mIconURL = "";
> >+        browser.mIconURL = browser.currentURI.prePath + "/favicon.ico";

> We can't do this here. Bug 515188 and the regression, bug 523369, tell us that
> we should use "browser.contentDocument.documentURIObject.prePath", but we can't
> in e10s. Therefore, we should leave it alone.
> 
> In order to set the default favicon, we may need to send back the
> documentURIObject.prePath or .spec in one of the other messages.

Thanks for pointing it.
Since I'll follow the one DOMLinkAdded message strategy from content to chrome and bind as many attributes as possible in the message this will go away. At first this hack was just a way to avoid adding an other attribute (prePath) on the initial message of my previous patch.

I'll provide a new patch tomorrow morning.
Attached patch Patch v0.2 (obsolete) — — Splinter Review 
Adress comments.
Attachment #445986 - Attachment is obsolete: true
Attachment #446229 - Flags: review?(webapps)
Attachment #446229 - Flags: review?(mark.finkle)
Attachment #445986 - Flags: review?(webapps)
Comment on attachment 446229 [details] [diff] [review]
Patch v0.2

>+  handleEvent: function(aEvent) {
>+    switch (aEvent.type) {
>+      case "DOMContentLoaded":
>+      {

Just curious about our style convention for this.  Since we use braces on the same lines for everything else, would it make sense here?  I'm happy with this style as long as it's a conscious decision that stays consistent across Fennec's codebase.

>+        // XXX We could probably send more here depending on our needs
>+        let json = {
>+          spec: documentURIObject.spec,
>+          prePath: documentURIObject.prePath
>+        };

Mark and I have different views on this point.  I'm concerned about putting too much on these objects without any real data that developers would use it.  My stance is that we should not go out of our way to add things we or anybody else may ever use.

>+      <field name="_searchEngines">[]</field>
>+      <property name="searchEngines"
>+                onget="return this._searchEngines"
>+                readonly="true"/>
>+

Does search engine stuff really belong on browser since it's a Fennec specific feature?  Mark?

>+              let event = document.createEvent("Events");
>+              event.initEvent("DOMTitleChanged", true, true);
>+              this.dispatchEvent(event);

We need to stop faking these events.  Devs can listen to the message.
Attachment #446229 - Flags: review?(webapps) → review-
(In reply to comment #6)
> (From update of attachment 446229 [details] [diff] [review])
> >+  handleEvent: function(aEvent) {
> >+    switch (aEvent.type) {
> >+      case "DOMContentLoaded":
> >+      {
> 
> Just curious about our style convention for this.  Since we use braces on the
> same lines for everything else, would it make sense here?  I'm happy with this
> style as long as it's a conscious decision that stays consistent across
> Fennec's codebase.

It looks like the convention is to put the brace on the same line as the control instruction.(https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide#Control_Structures)

I'll change for that.

> 
> >+        // XXX We could probably send more here depending on our needs
> >+        let json = {
> >+          spec: documentURIObject.spec,
> >+          prePath: documentURIObject.prePath
> >+        };
> 
> Mark and I have different views on this point.  I'm concerned about putting too
> much on these objects without any real data that developers would use it.  My
> stance is that we should not go out of our way to add things we or anybody else
> may ever use.

Since we are passing json I assume a copy of the data is done before being passed and this can be slow.
I'll remove the comment.

> 
> >+      <field name="_searchEngines">[]</field>
> >+      <property name="searchEngines"
> >+                onget="return this._searchEngines"
> >+                readonly="true"/>
> >+
> 
> Does search engine stuff really belong on browser since it's a Fennec specific
> feature?  Mark?

It is not fennec specific in my opinion since the top-right search box in firefox look for search engine of the current tab/website

> 
> >+              let event = document.createEvent("Events");
> >+              event.initEvent("DOMTitleChanged", true, true);
> >+              this.dispatchEvent(event);
> 
> We need to stop faking these events.  Devs can listen to the message.

I agree with that but I think this belong to another bug (no bug # yet) which correspond to the task of doing a message API for DOMTitleChanged and friends.
Attached patch Patch (obsolete) — — Splinter Review 
The patch addressed comments but still used searchEngines as a property of browser.xml.
Assignee: nobody → 21
Attachment #446229 - Attachment is obsolete: true
Attachment #446481 - Flags: review?(webapps)
Attachment #446481 - Flags: review?(mark.finkle)
Attachment #446229 - Flags: review?(mark.finkle)
Comment on attachment 446481 [details] [diff] [review]
Patch

>+    // listen content messages
>+    window.messageManager.addMessageListener("DOMLinkAdded", this);

Nit: could we change these references to just messageManager?  It really has nothing to do with the window object, and it it implies that these messages can "bubble up" somehow to the window.

Please file a follow up bug on DOMTitleChange events and their ilk.
Attachment #446481 - Flags: review?(webapps) → review+
Attachment #446481 - Flags: review?(mark.finkle) → review-
Comment on attachment 446481 [details] [diff] [review]
Patch

>diff -r 30c0f926f2e2 chrome/content/bindings/browser.js

>+        sendAsyncMessage("DOMContentLoaded", {
>+          spec: documentURIObject.spec,
>+          prePath: documentURIObject.prePath

Sending back spec should be enough. We can recreate a nsIURI in the parent chrome process. Sending back prePath is cheaper for this specific use-case, butwhy not send everything back? Using spec should be enough.

However, I think we should call the data "location", not "spec". DOMContentLoaded might send other data, so we can future-proof the JSON a little by not using "spec" and we use "location" in the webprogress onLocationChange message

>+        let json = {
>+          title: link.title,
>+          href: link.href,
>+          rel: link.rel,
>+          type: link.type,
>+          isRootDocument: ownerDoc.defaultView.frameElement ? false : true,

It's no secret that I hate isRootDocument. I wish we had a better way.

>diff -r 30c0f926f2e2 chrome/content/bindings/browser.xml

>+        <body><![CDATA[
>+          switch (aMessage.name) {
>+            case "DOMContentLoaded":
>+            {
>+              let data = aMessage.json;
>+              if (this.mIconURL == "")
>+                this.mIconURL = data.prePath + "/favicon.ico";

Since we are sending "location" back here, we will need to make an nsIURI from the string, then use prePath:

  let documentURI = gIOService.newURI(data.location, null, null);


I yielded to Ben and Matt about "window.addMessageEventListener", so let's drop the "window."

r- just for the DOMContentLoaded change, but I think this patch will be ready after that.
Attached patch Patch v0.3 (obsolete) — — Splinter Review 

        Attachment #446481 -
        Attachment is obsolete: true

        Attachment #446731 -
        Flags: review?(webapps)

        Attachment #446731 -
        Flags: review?(mark.finkle)

    
    

    
  
Comment on attachment 446731 [details] [diff] [review]
Patch v0.3

Looking good

        Attachment #446731 -
        Flags: review?(mark.finkle) → review+

        Attachment #446731 -
        Flags: review?(webapps) → review+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

    
    

    
  
Reopend because I need to adapt the code to mobile-browser!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  

      
      
      
      

        Attached patch
          
          
        wip v0.1 (obsolete)
        — 
          — Splinter Review
      

    


  
First draft to backport code from e10s to mobile-browser.

This has a initial strip down browser.xml and browser.js and there is probably too many stuff (notably for the web progress events) but I since these parts are needed for a near future I think we should keep it.

        Attachment #448777 -
        Flags: review?(mark.finkle)

    
    

    
  
Comment on attachment 448777 [details] [diff] [review]
wip v0.1

Should we wait for your DOM message API to land first?

    
    

    
  
(In reply to comment #16)
> (From update of attachment 448777 [details] [diff] [review])
> Should we wait for your DOM message API to land first?

Oh, this bug actually blocks bug 568092

    
    

    
  
(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 448777 [details] [diff] [review] [details])
> > Should we wait for your DOM message API to land first?
> 
> Oh, this bug actually blocks bug 568092

Yep, we don't have to wait for anything :)

    
    

    
  
Comment on attachment 448777 [details] [diff] [review]
wip v0.1

This patch has bitrotted a little. Do you want to update it first, before we land it?

        Attachment #448777 -
        Flags: review?(mark.finkle) → review+

    
    

    
  
Comment on attachment 446731 [details] [diff] [review]
Patch v0.3

This patch no longer applies cleanly.

I applied the browser.xml patch first.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch
        — 
          — Splinter Review
      

    


  
This patch is based on the one from bug 570588 and add a remote=false attribute to the browsers created in Fennec which activate the browser.xml binding.

I've commented a few properties of browser.xml where the code of mobile-browser need to be adapted.

        Attachment #448777 -
        Attachment is obsolete: true

        Attachment #449833 -
        Flags: review?(mark.finkle)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v0.4
        — 
          — Splinter Review
      

    


  
Patch updated on current trunk.

        Attachment #446731 -
        Attachment is obsolete: true

        Attachment #449878 -
        Flags: review?(mark.finkle)

        Attachment #449878 -
        Flags: review?(mark.finkle) → review+

    
    

    
  
m-b: http://hg.mozilla.org/mobile-browser/rev/0660d7f0d3e9
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 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: