Closed Bug 1272774 Opened 8 years ago Closed 6 years ago

some favicons are missing on about:debugging / tabs

Categories

(DevTools :: about:debugging, defect, P2)

defect

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: soeren.hentzschel, Assigned: jdescottes)

References

(Blocks 3 open bugs)

Details

Attachments

(6 files, 1 obsolete file)

Please have a look at the screenshot. Some favicons are missing (Google Docs, WordPress admin area).
Hi Sören, thank you for catching this so quickly! I believe this is because we use a hardcoded "/favicon.ico" file, instead of letting the website properly define its favicon via meta tags or a manifest file.

However, we definitely shouldn't show broken images like the ones in your screenshot, and instead show at least the default globe icon. Alex, could you please have a look at this?
Flags: needinfo?(poirot.alex)
Priority: -- → P1
I have some bandwidth to take the bug. 

I'd plan to use PlacesUtils.promiseFaviconLinkUrl to collect the correct favicon, and replace hardcode default icon path with `PlacesUtils.favicons.defaultFavicon.spec`

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesUtils.jsm#1680

Is that the right approach?
Assignee: nobody → gasolin
Flags: needinfo?(janx)
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Icon shows fine in WIP. Current implementation will trigger tabs render several times (setState), need some suggestion if it matters.
Flags: needinfo?(janx)
Attachment #8759544 - Flags: feedback?(janx)
Flags: needinfo?(poirot.alex)
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Thanks a lot for addressing this gasolin!

Alex would be the best person to review such a change.

That said, I tried your WIP patch and it works for me! (Favicons that previously looked broken are now displayed successfully). My two biggest worries are:

- Your approach doesn't use our devtools actors, so it probably won't work correctly for remote debugging devices when we enable them (e.g. maybe Firefox won't be able to retrieve the favicon of a web app on Fennec).

- The fetched icons are low-resolution, even though we display them at a larger size in about:debugging. Maybe there is a way to ask Firefox for a high-resolution favicon for a given website?
Attachment #8759544 - Flags: feedback?(janx) → review?(poirot.alex)
Attachment #8759544 - Flags: review?(poirot.alex)
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

https://reviewboard.mozilla.org/r/57538/#review54720

::: devtools/client/aboutdebugging/components/tabs/panel.js:52
(Diff revision 1)
>        tabs.forEach(tab => {
>          // FIXME Also try to fetch low-res favicon. But we should use actor
>          // support for this to get the high-res one (bug 1061654).
>          let url = new URL(tab.url);
>          if (url.protocol.startsWith("http")) {
> -          let prePath = url.origin;
> +          let promise = PlacesUtils.promiseFaviconLinkUrl(url);

Thanks for looking into this.

As Jan said, it would ideally be on the actor side.

Like in TabActor's form:
http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1075

Any attribute put on `response` object is going to be available here, on `tab` object.

Unfortunately form() is synchronous, which doesn't fit the PlaceUtils helper synchronousness.
But we already had to work around that limitation.
See `update()`, where you can introduce some async computation required in `form()`:
http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1056

Also, may be you could use the favicon xpcom (mozIAsyncFavicons) directly to prevent loading placeutils.jsm.

It would be great to add a test against the icon over here:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_tabs.js?force=1#39
newTabTarget is an instance of this react component:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/aboutdebugging/components/addons/target.js
Mak do you know if there's an iconservice to get high-res icon?
Flags: needinfo?(mak77)
(In reply to Fred Lin [:gasolin] from comment #7)
> Mak do you know if there's an iconservice to get high-res icon?

I will start working on bug 492172 soon (Likely before the end of June) since that's also a request for Activity Stream.  For now we don't have hi-res icons.
Flags: needinfo?(mak77)
Gasolin, Alex suggested in comment 6 to change the devtools TabActor to return a favicon URL.

It is important to use an iconservice through a devtools actor, which lives inside a target device, because when we'll be able to connect about:debugging to Fennec phones (e.g. via a USB cable), the actor will be able to use the device's iconservice instead of the iconservice from desktop Firefox. (Maybe the desktop's iconservice will not know the icon for a tab that's open on the device.)

Regarding which icon service to use, I guess you can use either PlacesUtils.promiseFaviconLinkUrl in the TabActor like you did in your previous patch, or use AsyncFavicons[0] directly as Alex suggested.

[0] https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/places/favicon.js#19

Regarding high-resolution icons, from Marco's comment 8, it looks like we don't know how to get these yet, so we'll have to keep the low-resolution icons in about:debugging for now until bug 492172 is fixed.

Thanks again for taking on this bug! Please let us know if you have any questions.
Flags: needinfo?(gasolin)
Thanks for explanation, I'm just back from the vacation and I'd like learn more about the actor in this workweek. See you soon :)
Status: NEW → ASSIGNED
Flags: needinfo?(gasolin)
after tracing code, I plan to move icon retrieving logics to the `onListTabs` function in actor/root.js 

https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/root.js#250

by add extra `then` clause in `onListTabs` function, we don't need change form() to promise in webbrowser.js 


The code will looks like

-   return reply;
- });
+   return promise.resolve(reply);
+ }).then(reply => { // find proper icon
+   // Filter out closed tabs (represented as `null`).
+   let tabs = reply.tabs.filter(tab => !!tab);
+   ...
+   reply.tabs = tabs;
+   return reply;
+ });

I may pipe another `then` clause to deal with the promise.all iconservice results

ex: 

+   return promise.resolve(reply);
+ }).then(reply => { // find proper icon
+   ...
+   return new promise.resolve([reply, promise.all(tabPromises)]);
+ }).then((...) => {
+   return reply;
+ });


Is that make sense to you?
Flags: needinfo?(janx)
Hi Gasolin, thanks for following up on this!

Moving the icon retrieving logic to `onListTabs` is an interesting idea, but I'm not sure we want to delay the tabs list response just to fetch all the icons: fetching them all might take some time, and maybe some callers of `onListTabs` don't actually need icons, so they shouldn't have to wait for them.

Alex, what do you think of Gasolin's idea in comment 11?
Flags: needinfo?(janx) → needinfo?(poirot.alex)
Yes. RootActor should not contain such logic. This actor is meant to be generic and focus on providing access to other actors.

Fetching and exposing icons should be part of the TabActor, from webbrowser.js.
Now, we have this limitation on form() which is supposed to be synchronous.
I mentioned the update() method, which I thought was called in every case, but it looks like it is only called when we fetch an already created actor.
  https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#369
But may be that's easier to convert form() to be async for tab actors? Would it require a big refactoring? Or if that require changing many callsites, may be we could let form() as-is and expose another method returning a promise and which would add additional data on the form like the icon?

Otherwise I don't have a strong opinion about fetching the icons when doing listTabs.
Here I mostly care about not polluting RootActor with tab things.

One way to mitigate all concerns could be to expose a getIcon request on TabActor and call it for every tab.
Another option could be to pass options to listTabs, in order to define how much data you would like to fetch, but that may require tweaking various callsites.
Flags: needinfo?(poirot.alex)
Hmm, I'll try to assign the icon service promise for tab.icon in webbrowser.js:form, then resolve the promise in client. Therefore it won't block the tabList retrieval process.
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57538/diff/1-2/
Attachment #8759544 - Attachment description: Bug 1272774 - show favicons on about:debugging / tabs via PlacesUtils.promiseFaviconLinkUrl; → Bug 1272774 - show favicons on about:debugging via favicons service;
In WIP I moved favicons related code to devtools/server/actors/webbrowser.js and do the lazy icon retrieving via response.iconPromise


Though in WIP I always encounter `favicons not found` error (same issue when I use PlaceUtils) in webbrowser.js 

Do I need any special settings?
Flags: needinfo?(janx)
Alex, is this the approach you were expecting from comment 6 and comment 13? Please have a look at the attached WIP patch.
Flags: needinfo?(janx) → needinfo?(poirot.alex)
https://reviewboard.mozilla.org/r/57538/#review58112

It looks like you have issues figuring your way into our codebase.
Here is a working solution:

From webbrowser.js, add a getIcon method on TabActor.prototype, which do the call to favicons.getFaviconURLForPage. One actor method *can* return a promise if the value can't be fetched immediately.
(but the resolved value can't contain promises, it should be only pure js objects)
  TabActor.prototype.getIcon = function () {
    return new Promise(resolve => {
      ...
      resolve(theIconUrl);
    });
  };
Then, you need to register the new getIcon method in TabActor.prototype.requestTypes.

From panel.js, you should be able to do that:
    let packet = {
      to: tab.actor,
      type: "getIcon"
    };
    return this.props.client.request(packet, response => {
      // response.icon should be the icon
      ...
    });

::: devtools/server/actors/webbrowser.js:1103
(Diff revision 2)
> +      if (url.protocol.startsWith("http")) {
> +        if (!(url instanceof Ci.nsIURI)) {
> +           url = NetUtil.newURI(url);
> +        }
> +
> +        response.iconPromise = new promise(resolve => {

Here you should be using the DOM Promises, with a capital P:
"new Promise"

promise, with a lowercase p is the defer version, used like this:
let defer = promise.defer();
someasyncstuff(function () {
  defer.resolve(resolvedValue);
});
return defer.promise;

::: devtools/server/actors/webbrowser.js:1112
(Diff revision 2)
> +              resolve(uri);
> +            } else {
> +              resolve(favicons.defaultFavicon.spec);
> +            }
> +          });
> +        });

Also, you are aware that this code runs as an actor, are you?
Actors are run in a totally different scope and values returned by the actor are stringified to be pipe through a TCP protocol. Only primitive data type can be returned: pure js objects, strings, numbers, booleans, ...
But no function, nor any non-pure-js-object like promises, DOM nodes, ...

So here, iconPromise won't work. the code in panel.js is not going to receive the promise.
Flags: needinfo?(poirot.alex)
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57538/diff/2-3/
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Thank you Alex, now I know how to request server data from the client side (via client.request).

The WIP can got icon successfully from devtool/server. Will add tests accordingly.
Attachment #8759544 - Flags: feedback?(poirot.alex)
https://reviewboard.mozilla.org/r/57538/#review58916

::: devtools/client/aboutdebugging/components/tabs/panel.js:51
(Diff revision 3)
> -        // FIXME Also try to fetch low-res favicon. But we should use actor
> -        // support for this to get the high-res one (bug 1061654).
> -        let url = new URL(tab.url);
> -        if (url.protocol.startsWith("http")) {
> -          let prePath = url.origin;
> -          let idx = url.pathname.lastIndexOf("/");
> +        let packet = {
> +          to: tab.actor,
> +          type: "getFavicon"
> +        };
> +
> +        this.props.client.request(packet, response => {

Note that request() also returns a promise which resolves the response object.

It may be better to update state only once all icons are fetched.

::: devtools/server/actors/webbrowser.js:1236
(Diff revision 3)
> +  // support for this to get the high-res one (bug 1061654).
> +  onGetFavicon() {
> +    let url = new URL(this.url);
> +    if (url.protocol.startsWith("http")) {
> +      if (!(url instanceof Ci.nsIURI)) {
> +         url = NetUtil.newURI(url);

No need to import NetUtil for this, you can do:
Services.io.newURI(url, null, null)

::: devtools/server/actors/webbrowser.js:1245
(Diff revision 3)
> +        favicons.getFaviconURLForPage(url, uri => {
> +          if (uri) {
> +            uri = favicons.getFaviconLinkForIcon(uri);
> +            resolve(uri);
> +          } else {
> +            resolve(favicons.defaultFavicon.spec);

it looks like you resolve a nsIURI object in the first resolve() and a string in the second.

::: devtools/server/actors/webbrowser.js:1255
(Diff revision 3)
> +          "from": this.actorID,
> +          "icon": iconNsIURI.spec
> +        };
> +      }).catch(err => {
> +        console.log(err);
> +      });

I'm not sure this catch() is any useful. The error should be prompted anyway.
Comment on attachment 8759544 [details]
Bug 1272774 - show favicons on about:debugging via favicons service;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57538/diff/3-4/
Attachment #8759544 - Flags: feedback?(poirot.alex)
Addressed issues commented in comment 21, use Promise.all to update icons at once.

The WIP can show icon successfully, though if I switch tab from about:debugging to other web page(ex twitter) then switch back to about:debugging page, the following error occurred in console and the tabs shows `Nothing yet` message.

> error occurred while processing 'getFavicon: ReferenceError: favicons is not defined
> Stack: onGetFavicon/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:1239:9
> onGetFavicon@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webbrowser.js:1238:14
> onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1648:15
> ChildDebuggerTransport.prototype.receiveMessage@resource://gre/modules/commonjs/toolkit/loader.js -> > resource://devtools/shared/transport/transport.js:743:7
> Line: 1239, column: 9

Not sure why got `favicons is not defined` while loading normal web page,
Is there a way to detour that?
Flags: needinfo?(poirot.alex)
https://reviewboard.mozilla.org/r/57538/#review59038

::: devtools/server/actors/webbrowser.js:32
(Diff revision 4)
>  loader.lazyRequireGetter(this, "ProcessActorList", "devtools/server/actors/process", true);
>  loader.lazyImporter(this, "AddonManager", "resource://gre/modules/AddonManager.jsm");
>  loader.lazyImporter(this, "ExtensionContent", "resource://gre/modules/ExtensionContent.jsm");
> -
> +XPCOMUtils.defineLazyServiceGetter(this, "favicons",
> +                                   "@mozilla.org/browser/favicon-service;1",
> +                                   "mozIAsyncFavicons");

The issue you are hitting is that this XPCOM doesn't work in the child process. it works only in the parent process. That's unfortunate :/

This webbrowser.js file is evaluated in the process where the targeted document lives.
For about: pages like about:debugging, about:addons, ... it lives in the parent process and works fine.
But when you open a tab and switch to mozilla.org, the tab is going to live into the content process. The TabActor for this tab is also going to live in the content process.

You might be able to work around that with a patch from bug 1240907 (the second patch which merge BrowserTabActor and RemoteBrowserTabActor). This patch already computes some TabActor fields in the parent process. Look into BrowserTabActor.form(). But again, it forces to put the icon in the form. form() is synchronous, but you can compute async stuff for it in connect() and update() methods which are async.

Given how uterly complex this ends up, it may be more reasonable to keep this code running on the client side... (like your first patch) It would be ok to do so if you can confirm that the favicon service is able to fetch icons for websites we never visited on Firefox.

Sorry for the roadblocks you are hitting, I would never have thought fetching favicons would be that complex!
> if you can confirm that the favicon service is able to fetch icons for websites we never visited on Firefox

Not sure what does that mean?
about:debugging is about to work with remote devices like Firefox for Android.
The actor is going to run on the Android device, while about:debugging runs in your desktop Firefox.
So tabs are going to be open and visited on Android, its favicon service is going to know about these tabs. But about:debugging, on desktop, won't. I have not idea how the favicon service behaves when you task about URL you never visited.
Flags: needinfo?(poirot.alex)
Hum. Does this service works at all?

I get null `uri` when executing that in the browser console:
Favicons.getFaviconDataForPage(Services.io.newURI("https://google.com/", null, null), uri => console.log("uri",uri))
> Hum. Does this service works at all?

Yes, I can still get icon uris with issue described in comment 23 though.


Is there a proper way to do the feature detection on server side? 

> if(!favicons) { // which seems not work
>   return ...
> }

therefore we can ignore the related call in child process?
Since I moved my priority to track#2 tasks,
 I'd reset assignee myself from this issue to not block the progress.

feel free to pick up the WIP patch
Assignee: gasolin → nobody
Status: ASSIGNED → NEW
Benoit, you said you might be interested in picking up this bug.

If that's still the case, please have a look at comment 6 and comment 13, where Alex explains how a TabActor method could provide a high-resolution favicon for a tab (note: it's probably better to have a separate `getIcon()` async actor method, rather than making all `form()` calls wait for icons).

Please feel free to take over Gasolin's WIP patch, or you can also take a different approach if you prefer.
Flags: needinfo?(bchabod)
(In reply to Jan Keromnes [:janx] from comment #30)
> Benoit, you said you might be interested in picking up this bug.
> 
> If that's still the case, please have a look at comment 6 and comment 13,
> where Alex explains how a TabActor method could provide a high-resolution
> favicon for a tab (note: it's probably better to have a separate `getIcon()`
> async actor method, rather than making all `form()` calls wait for icons).
> 
> Please feel free to take over Gasolin's WIP patch, or you can also take a
> different approach if you prefer.

Yes, this bug seems interesting!
I've added it to my backlog, and I'll try to work on it as soon as I can.
I'll keep you guys updated
Flags: needinfo?(bchabod)
Any chance this will get fixed? If it's too difficult to fetch the favicons showing no favicons at all would still be better than all the "broken image" images because, well, it looks broken… ;)
ni? :bigben for comment 32 because one year ago you said:

> and I'll try to work on it as soon as I can. I'll keep you guys updated

;)
Flags: needinfo?(be.chabod)
Sorry Sören, I was planning on fixing this bug during my internship but I ran out of time.
I have other projects now and I won't be working on this or any Firefox-related development in the near future.

Feel free to pick it up or to suggest it to someone!
Flags: needinfo?(be.chabod)
If this is still considered a P1 bug, can we get it moving? It looks like a lot of work has already been done, it would be too bad wasting it.
Fred: what's left here? Can you describe the remaining steps so someone else can pick it up?
Flags: needinfo?(gasolin)
Re-prioritizing as P2. Broken icons look bad but don't prevent using the feature.
Priority: P1 → P2
Based on comment 24, we could continue with the client-side approach in my origin patch
https://reviewboard.mozilla.org/r/57538/diff/1/

And need to make sure tabs icon shows correctly from Android(fennec) (As described in Comment 26)
Flags: needinfo?(gasolin)
Blocks: 1390643
Blocks: 1404663
Blocks: 1390642
Blocks: 1412596
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
(In reply to Marco Bonardo [::mak] from comment #8)
> (In reply to Fred Lin [:gasolin] from comment #7)
> > Mak do you know if there's an iconservice to get high-res icon?
> 
> I will start working on bug 492172 soon (Likely before the end of June)
> since that's also a request for Activity Stream.  For now we don't have
> hi-res icons.

Mak: I see that Bug 492172 is closed but I'm not sure which API/service we should use for high resolution icons?
Flags: needinfo?(mak77)
How do you set icons currently?

Either use the idl API on mozIAsyncFavicons (getFaviconXXXForPage) or set the image source to
PlacesUtils.urlWithSizeRef(window, "page-icon:" + page_url, your_preferred_icon_size);
If the icon size you wish is 16px (the UI default), just use "page-icon:page_url".
The page-icon method is usually simpler if your page is chrome privileged.

Note the favicons service for now doesn't have favicons in PB mode.
Flags: needinfo?(mak77)
Thanks for the quick answer.

> How do you set icons currently?

At the moment we just fetch them by guessing the favicon.ico url. But when trying with PlacesUtils.promiseFaviconLinkUrl, I got small resolution icons for some websites, hence my question. 

Just tried using PlacesUtils.urlWithSizeRef, and it seems to work perfectly!
you can also pass a size param to the APIs and they will "try" to fulfill your request. In that case the size you pass must be good for the size you wish to show and the current device pixels (see the implementation of urlWithSizeRef eventually, it's trivial).
promiseFaviconLinkUrl has not been updated yet, I should check the consumer, maybe we could even remove it.
Attachment #8934193 - Flags: review?(poirot.alex)
My import is a bit unclean, will fix that before requesting for review. 

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d31b41969523008b6bb61b786bca67b6fc68662
Comment on attachment 8934193 [details]
Bug 1272774 - allow listTabs to return favicon data from PlacesUtils;

https://reviewboard.mozilla.org/r/205136/#review211024

This patch fixes the broken icon, but also, it makes all icons be the global/default one when connecting to a remote.
Is that what you intent to do?
The favicon service only works for URLs you already visited, so unless you already visited the website your are debugging on the remote device, the icon will be the default one.

If you want to proceed with that behavior, you can wontfix bug 1061654.
Attachment #8934193 - Flags: review?(poirot.alex)
Good point. We could do this in two steps and first have about:debugging#tabs icons work for local debugging and have a follow-up to address the remote debugging case.

I'll give a shot to an actor-side solution first and see how complex it looks.
Hitting the same issues as Fred, with favicon service that can't run in the content process. 

In Comment 24 you mention that some fields of the browser tab actor are computed in the parent process. I assume this is about "url" and "title". But looking at the code, I don't see a clear pattern for communicating with the parent process from an actor. Can you share more information?
Flags: needinfo?(poirot.alex)
In the meantime, I was pointed at http://docs.firefox-dev.tools/backend/actor-e10s-handling.html which seems to cover this use case.
This doc is good for communicating from child to parent. But if you are around TabActor.form, you are in a special spot. Which already involve code running both in parent and content.
This module runs in parent process. Update is called every time the client fetches tab actor 's form. It runs in parent.
https://searchfox.org/mozilla-central/source/devtools/server/actors/webbrowser.js#739

It listen for messages sent by the child to update url/title.
Child send messages from here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/childtab.js#80

It would be easy to use the favicon from webbrowser.js. Do you think putting the icon in the tab actor form would work for about debugging? The only issue is that it is going to significantly increase the size of it. We also fetch the form for regular toolbox where we don't need that data.
Flags: needinfo?(poirot.alex)
Thanks for the info. I had not really understood the relationships between the BrowserTabActor, the ContentActor and the TabActor!

Until now I was focusing on the TabActor (tab.js) and I added a getFavicon method on the tab actor that asks the parent process to compute the base64 png for the favicon. Downsides are that I have to call it for every tab, plus I have to add the boilerplate code to perform the child/parent communication. But at least it's only executed when needed.

Can we call specific methods on the BrowserTabActor (or on the browser tab list) from the client side? I am not sure if the actor id I have in aboutdebugging for a tab actor corresponds to the BrowserTabActor or to the ContentActor. I have seen the comment "All RDP packets get forwarded using the message manager." in the BrowserTabActor doc but I couldn't find where/how this was achieved.

Anyway, knowing all of that, what about adding an options parameter to listTabs in order to ask for favicons in the BrowserTabActor form?
Comment on attachment 8934193 [details]
Bug 1272774 - allow listTabs to return favicon data from PlacesUtils;

this patch implements what I mentioned in my previous comment: optional argument in listTabs.
Attachment #8934193 - Flags: feedback?(poirot.alex)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #51)
> Until now I was focusing on the TabActor (tab.js) and I added a getFavicon
> method on the tab actor that asks the parent process to compute the base64
> png for the favicon. Downsides are that I have to call it for every tab,
> plus I have to add the boilerplate code to perform the child/parent
> communication. But at least it's only executed when needed.
> 
> Can we call specific methods on the BrowserTabActor

No. BrowserTabActor isn't registered to any DebuggerConnection (via DebuggerConnection.addActor).
BrowserTabActor only provides ContentActor's forms for RootActor.listTabs request.
You can see BrowserTabActor as internal implementation of listTabs, but not really as an actor.

> (or on the browser tab list) from the client side?

Same, BrowserTabList is only an implementation detail of listTabs request, this isn't an actor on its own.

> I am not sure if the actor id I have in aboutdebugging for a tab actor corresponds to the BrowserTabActor or to the
> ContentActor.

To the ContentActor.

> I have seen the comment "All RDP packets get forwarded using
> the message manager." in the BrowserTabActor doc but I couldn't find
> where/how this was achieved.

If you want to understand more, you have to go down into DebuggerServer internals.
The very precise spot where we forward messages between parent and child is here:
https://searchfox.org/mozilla-central/source/devtools/server/main.js#1754-1767
But this is very generic, there is nothing specific to BrowserTabActor.
The only special thing that is very specific to BrowserTabActor<->ContentActor
is "debug:form" message, that is used to communicated form's updated between these two classes.

> Anyway, knowing all of that, what about adding an options parameter to
> listTabs in order to ask for favicons in the BrowserTabActor form?

Sounds good to me.
Attachment #8934193 - Flags: feedback?(poirot.alex) → feedback+
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83c09bdf2871b533944abba3813f4d3be0db4cca

I couldn't find an easy way to add an optional parameter to listTabs, so I ended up adding a new method listTabsWithFavicons. Let me know if there is a cleaner way of doing that.
Comment on attachment 8934193 [details]
Bug 1272774 - allow listTabs to return favicon data from PlacesUtils;

https://reviewboard.mozilla.org/r/205136/#review218542

(In reply to Julian Descottes [:jdescottes][:julian] from comment #59)
> I couldn't find an easy way to add an optional parameter to listTabs, so I
> ended up adding a new method listTabsWithFavicons. Let me know if there is a
> cleaner way of doing that.

Where is that difficult? Your previous patch was able to do that, wasn't it?
The issue with the current patch is that tabs debugging won't work with older runtimes.

If that's because of 'DebuggerClient.requester' havinging issues with optional parameters,
you may definite listTabs like 'getTab', with a function calling 'request' method manually.

::: devtools/server/actors/webbrowser.js:793
(Diff revision 4)
> -      return this._deferredUpdate.promise;
> +    });
> +
> +    this._form = form;
> +    if (this.options.favicons) {
> +      this._form.favicon = await this.getFaviconData();
>      }

nit: It would have been great to fetch the favicon only in once place and not from both connect and update.
`form` would be a natural placeholder, but can't return a promise...
Attachment #8934193 - Flags: review?(poirot.alex)
Comment on attachment 8941789 [details]
Bug 1272774 - remove unused update() method on tab actor;

https://reviewboard.mozilla.org/r/211706/#review218548

Thanks for identifying and cleaning this up!
Attachment #8941789 - Flags: review?(poirot.alex) → review+
Comment on attachment 8941790 [details]
Bug 1272774 - remove irrelevant cleanup comment;

https://reviewboard.mozilla.org/r/211708/#review218550
Attachment #8941790 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #60)
> Comment on attachment 8934193 [details]
> Bug 1272774 - allow listTabs to return favicon data from PlacesUtils;
> 
> https://reviewboard.mozilla.org/r/205136/#review218542
> 
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #59)
> > I couldn't find an easy way to add an optional parameter to listTabs, so I
> > ended up adding a new method listTabsWithFavicons. Let me know if there is a
> > cleaner way of doing that.
> 
> Where is that difficult? Your previous patch was able to do that, wasn't it?

listTabs was working from about debugging but failing everywhere where listTabs was called with a callback argument. Try was completely busted with my previous patch.

> The issue with the current patch is that tabs debugging won't work with
> older runtimes.
> 

Right I keep forgetting that about debugging "can" connect to other runtimes now.

> If that's because of 'DebuggerClient.requester' havinging issues with
> optional parameters,
> you may definite listTabs like 'getTab', with a function calling 'request'
> method manually.
> 

Yes the issue is that requester always adds a "callback" as optional last parameter. Adding my options parameter, the callback is expected to be in 2nd position. I can do something similar to getTab except that in this case it is a bit dirty. The argument is either an object or a function, and has a different role depending on its type.

Or I can migrate all callers that use the callback argument. It seems to mostly be test code.

> ::: devtools/server/actors/webbrowser.js:793
> (Diff revision 4)
> > -      return this._deferredUpdate.promise;
> > +    });
> > +
> > +    this._form = form;
> > +    if (this.options.favicons) {
> > +      this._form.favicon = await this.getFaviconData();
> >      }
> 
> nit: It would have been great to fetch the favicon only in once place and
> not from both connect and update.
> `form` would be a natural placeholder, but can't return a promise...

I assume this is about code duplication. I was bothered by this too, thought about adding an `async function wrapForm` or something like that to mutualize a bit more.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #64)
> > If that's because of 'DebuggerClient.requester' havinging issues with
> > optional parameters,
> > you may definite listTabs like 'getTab', with a function calling 'request'
> > method manually.
> > 
> 
> Yes the issue is that requester always adds a "callback" as optional last
> parameter. Adding my options parameter, the callback is expected to be in
> 2nd position.

Ah ok, got it...

> I can do something similar to getTab except that in this case
> it is a bit dirty. The argument is either an object or a function, and has a
> different role depending on its type.

That's an option. That wouldn't be the only place we do that...

> Or I can migrate all callers that use the callback argument. It seems to
> mostly be test code.

Ideally, in theory, we should stop using callbacks but promises to get the returned value.
I did such refactoring for a couple of APIs but not all of them. bug 1237598 is about that.

> I assume this is about code duplication. I was bothered by this too, thought
> about adding an `async function wrapForm` or something like that to
> mutualize a bit more.

It would be something to do for sure if we introduce any other attribute like favicon,
but just for favicon, I imagine I have the same hesitation.
I ended up removing all usage of the callback. Try looks good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=802014d04873f72b605bb04d82dc6a188ee42782
Comment on attachment 8942702 [details]
Bug 1272774 - migrate all listTabs() callers to use promise;

https://reviewboard.mozilla.org/r/212968/#review218606

Thanks for the cleanup!

::: devtools/client/shared/test/test-actor-registry.js:23
(Diff revision 1)
>    // Register a test actor that can operate on the remote document
>    exports.registerTestActor = Task.async(function* (client) {
>      // First, instanciate ActorRegistryFront to be able to dynamically register an actor
>      let deferred = defer();
> -    client.listTabs(deferred.resolve);
> +    client.listTabs().then(deferred.resolve);
>      let response = yield deferred.promise;

This can be simplified to:
let response = yield client.listTabs();

::: devtools/shared/security/tests/unit/test_encryption.js:20
(Diff revision 1)
>  function connectClient(client) {
>    let deferred = defer();
>    client.connect(() => {
> -    client.listTabs(deferred.resolve);
> +    client.listTabs().then(deferred.resolve);
>    });
>    return deferred.promise;

This can be simplified to:
return client.connect(() => client.listTabs());
Attachment #8942702 - Flags: review?(poirot.alex) → review+
Comment on attachment 8934193 [details]
Bug 1272774 - allow listTabs to return favicon data from PlacesUtils;

https://reviewboard.mozilla.org/r/205136/#review218612

Looks good, works great. I tested the remote capabilities of about:debugging :)

::: devtools/client/aboutdebugging/components/tabs/Panel.js
(Diff revision 5)
> -          if (idx === -1) {
> -            prePath += url.pathname;
> -          } else {
> -            prePath += url.pathname.substr(0, idx);
> -          }
> -          tab.icon = prePath + "/favicon.ico";

I was only wondering if we want to keep this code for old runtime.
With the current patch all icons are the default one, the globe, when connecting to old runtime.
But would mean this bug would still occur when connecting against them...
Attachment #8934193 - Flags: review?(poirot.alex) → review+
Attachment #8759544 - Attachment is obsolete: true
Comment on attachment 8934193 [details]
Bug 1272774 - allow listTabs to return favicon data from PlacesUtils;

https://reviewboard.mozilla.org/r/205136/#review218612

> I was only wondering if we want to keep this code for old runtime.
> With the current patch all icons are the default one, the globe, when connecting to old runtime.
> But would mean this bug would still occur when connecting against them...

Thanks for the reviews! Will wait until after merge day to land, in case I missed something with the listTabs cleanup.
I think that it's fine to show the default globe for old runtimes. Keeps the code relatively simple and the feature is still working.
Comment on attachment 8942702 [details]
Bug 1272774 - migrate all listTabs() callers to use promise;

https://reviewboard.mozilla.org/r/212968/#review221202


Static analysis found 6 defects in this patch.
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/debugger/test/mochitest/browser_dbg_globalactor.js:26
(Diff revision 3)
>    gClient = new DebuggerClient(transport);
>    gClient.connect().then(([aType, aTraits]) => {
>      is(aType, "browser",
>        "Root actor should identify itself as a browser.");
>  
> -    gClient.listTabs(aResponse => {
> +    gClient.listTabs().then(aResponse => {

Error: Parameter 'aResponse' uses Hungarian Notation, consider using 'response' instead. [eslint: mozilla/no-aArgs]

::: devtools/client/debugger/test/mochitest/browser_dbg_multiple-windows.js:48
(Diff revision 3)
>    let deferred = promise.defer();
>  
>    gNewTab = aTab;
>    ok(!!gNewTab, "Second tab created.");
>  
> -  gClient.listTabs(aResponse => {
> +  gClient.listTabs().then(aResponse => {

Error: Parameter 'aResponse' uses Hungarian Notation, consider using 'response' instead. [eslint: mozilla/no-aArgs]

::: devtools/client/debugger/test/mochitest/browser_dbg_multiple-windows.js:78
(Diff revision 3)
>  
>    let isActive = promise.defer();
>    let isLoaded = promise.defer();
>  
>    promise.all([isActive.promise, isLoaded.promise]).then(() => {
> -    gClient.listTabs(aResponse => {
> +    gClient.listTabs().then(aResponse => {

Error: Parameter 'aResponse' uses Hungarian Notation, consider using 'response' instead. [eslint: mozilla/no-aArgs]

::: devtools/client/debugger/test/mochitest/browser_dbg_multiple-windows.js:118
(Diff revision 3)
>  
>  function testFocusFirst() {
>    let deferred = promise.defer();
>  
>    once(window.content, "focus").then(() => {
> -    gClient.listTabs(aResponse => {
> +    gClient.listTabs().then(aResponse => {

Error: Parameter 'aResponse' uses Hungarian Notation, consider using 'response' instead. [eslint: mozilla/no-aArgs]

::: devtools/client/debugger/test/mochitest/browser_dbg_multiple-windows.js:145
(Diff revision 3)
>  
>  function continue_remove_tab(deferred)
>  {
>    removeTab(gNewTab);
>  
> -  gClient.listTabs(aResponse => {
> +  gClient.listTabs().then(aResponse => {

Error: Parameter 'aResponse' uses Hungarian Notation, consider using 'response' instead. [eslint: mozilla/no-aArgs]

::: devtools/client/debugger/test/mochitest/head.js:164
(Diff revision 3)
>  }
>  
>  function getTabActorForUrl(aClient, aUrl) {
>    let deferred = promise.defer();
>  
> -  aClient.listTabs(aResponse => {
> +  aClient.listTabs().then(aResponse => {

Error: Parameter 'aResponse' uses Hungarian Notation, consider using 'response' instead. [eslint: mozilla/no-aArgs]
Dropped all the automated review issues as they concern ignored file. The issue was already reported and fixed at https://github.com/mozilla-releng/services/issues/788
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab564531dda9
migrate all listTabs() callers to use promise;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/357cf743b582
allow listTabs to return favicon data from PlacesUtils;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/22254eef5a3a
remove unused update() method on tab actor;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/93aa8ac59522
remove irrelevant cleanup comment;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/ec6bd22c4d00
rename actor/childtab.js to actor/content.js to match ContentActor;r=ochameau
I have reproduced this bug with Nightly 49.0a1 (2016-05-13) on Windows 10, 64 Bit!

This bug's fix is verified with latest Beta!

Build ID   : 20180402175344
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
QA Whiteboard: [testday-20180413]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: