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)
DevTools
about:debugging
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)
522.22 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ochameau
:
review+
|
Details |
Please have a look at the screenshot. Some favicons are missing (Google Docs, WordPress admin area).
Comment 1•8 years ago
|
||
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
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/57538/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57538/
Comment 4•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(poirot.alex)
Comment 5•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8759544 -
Flags: review?(poirot.alex)
Comment 6•8 years ago
|
||
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
Comment 7•8 years ago
|
||
Mak do you know if there's an iconservice to get high-res icon?
Flags: needinfo?(mak77)
Comment 8•8 years ago
|
||
(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)
Comment 9•8 years ago
|
||
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)
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
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)
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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;
Comment 16•8 years ago
|
||
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)
Comment 17•8 years ago
|
||
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)
Comment 18•8 years ago
|
||
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.
Updated•8 years ago
|
Flags: needinfo?(poirot.alex)
Comment 19•8 years ago
|
||
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 20•8 years ago
|
||
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)
Comment 21•8 years ago
|
||
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 22•8 years ago
|
||
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)
Comment 23•8 years ago
|
||
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)
Comment 24•8 years ago
|
||
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!
Comment 25•8 years ago
|
||
> 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?
Comment 26•8 years ago
|
||
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)
Comment 27•8 years ago
|
||
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))
Comment 28•8 years ago
|
||
> 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?
Comment 29•8 years ago
|
||
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
Comment 30•8 years ago
|
||
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)
Comment 31•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Blocks: top-aboutdebugging-bugs
Reporter | ||
Comment 32•7 years ago
|
||
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… ;)
Reporter | ||
Comment 33•7 years ago
|
||
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)
Comment 34•7 years ago
|
||
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)
Comment 35•6 years ago
|
||
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)
Assignee | ||
Comment 37•6 years ago
|
||
Re-prioritizing as P2. Broken icons look bad but don't prevent using the feature.
Priority: P1 → P2
Comment 38•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee | ||
Comment 39•6 years ago
|
||
(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)
Comment 40•6 years ago
|
||
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)
Assignee | ||
Comment 41•6 years ago
|
||
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!
Comment 42•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8934193 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 44•6 years ago
|
||
My import is a bit unclean, will fix that before requesting for review. Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d31b41969523008b6bb61b786bca67b6fc68662
Comment hidden (mozreview-request) |
Comment 46•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 47•6 years ago
|
||
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.
Assignee | ||
Comment 48•6 years ago
|
||
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)
Assignee | ||
Comment 49•6 years ago
|
||
In the meantime, I was pointed at http://docs.firefox-dev.tools/backend/actor-e10s-handling.html which seems to cover this use case.
Comment 50•6 years ago
|
||
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)
Assignee | ||
Comment 51•6 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 53•6 years ago
|
||
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)
Comment 54•6 years ago
|
||
(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.
Updated•6 years ago
|
Attachment #8934193 -
Flags: feedback?(poirot.alex) → feedback+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 59•6 years ago
|
||
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 60•6 years ago
|
||
mozreview-review |
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 61•6 years ago
|
||
mozreview-review |
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 62•6 years ago
|
||
mozreview-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+
Comment 63•6 years ago
|
||
mozreview-review |
Comment on attachment 8941791 [details] Bug 1272774 - rename actor/childtab.js to actor/content.js to match ContentActor; https://reviewboard.mozilla.org/r/211710/#review218552 You should also rename it from here: https://searchfox.org/mozilla-central/source/devtools/docs/backend/actor-hierarchy.md#51 https://searchfox.org/mozilla-central/source/devtools/server/actors/webbrowser.js#746
Attachment #8941791 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 64•6 years ago
|
||
(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.
Comment 65•6 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 71•6 years ago
|
||
I ended up removing all usage of the callback. Try looks good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=802014d04873f72b605bb04d82dc6a188ee42782
Comment 72•6 years ago
|
||
mozreview-review |
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 73•6 years ago
|
||
mozreview-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+
Assignee | ||
Updated•6 years ago
|
Attachment #8759544 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 79•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 85•6 years ago
|
||
rebased, sent to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=b06814bbc646a9d796bba0a25c1c71f929b13900 will land when green.
Comment 86•6 years ago
|
||
mozreview-review reviewboard-spam |
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]
Assignee | ||
Comment 87•6 years ago
|
||
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
Comment 88•6 years ago
|
||
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
Comment 89•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ab564531dda9 https://hg.mozilla.org/mozilla-central/rev/357cf743b582 https://hg.mozilla.org/mozilla-central/rev/22254eef5a3a https://hg.mozilla.org/mozilla-central/rev/93aa8ac59522 https://hg.mozilla.org/mozilla-central/rev/ec6bd22c4d00
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 90•6 years ago
|
||
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]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•