Closed
Bug 1305528
Opened 8 years ago
Closed 7 years ago
Remove ignoreEvent from API events for which it is inappropriate
Categories
(WebExtensions :: General, defect, P3)
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: adam.schenker, Assigned: shatur, Mentored)
Details
(Keywords: dev-doc-complete, Whiteboard: triaged[good first bug])
Attachments
(2 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.116 Safari/537.36 Steps to reproduce: Loaded a WebExtensions extension (attached) that uses chrome.tabs.onReplaced.addListener. Actual results: Received a message in the Browser Console saying this function is not implemented: In add-on a478fc5d00fa473c6d32e67ca6104e0c78d5e613@temporary-addon, attempting to use listener "tabs.onReplaced", which is unimplemented. Expected results: No warning message should be generated. Documentation on this API states it is supported beginning in Firefox 45.0: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onReplaced
Reporter | ||
Updated•8 years ago
|
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Reporter | ||
Comment 1•8 years ago
|
||
The upload uses browser.tabs.onReplaced.addListener but the same behavior happens with chrome.tabs.onReplaced.addListener.
Comment 2•8 years ago
|
||
I am also unable to reproduce this. Can you please confirm which platform you are on, and the version of your OS and Firefox?
Component: Untriaged → WebExtensions: General
Flags: needinfo?(adam.schenker)
Product: Firefox → Toolkit
Version: 49 Branch → unspecified
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg] from comment #2) > I am also unable to reproduce this. Can you please confirm which platform > you are on, and the version of your OS and Firefox? I'm on Mac OS X 10.10.5. I've tried both the current production version (49.0.1) and the latest Nightly of 52.0a1 and both give the same message.
Comment 4•8 years ago
|
||
I can't reproduce either in Firefox 49, OS X 10.10.5.
Reporter | ||
Comment 5•8 years ago
|
||
The warning message gives line 599:89 in resource://gre/modules/Extension.jsm for Firefox 49.0.1 if that is helpful. Let me know if there's anything else I can do to help.
Comment 6•8 years ago
|
||
Ah, this is just a warning, and it is expected. There is a discussion going on about whether we should or should not be issuing this warning. Perhaps we can change this bug to request that a warning not be issued in this situation.
Comment 7•8 years ago
|
||
When we use ignoreEvent, a warning is issued in the Browser Console when an extension calls addListener or removeListener on the event. There are cases when we want this warning issued, for example if we have not yet implemented an event that could be implemented in the future. There are also cases when it is unnecessary to issue a warning, because the event could never fire from the browser. The onTabReplaced events are the latter, as Firefox does not have the concept of replacing a tab. Therefore the calls to ignoreEvent in ext-tabs.js [1] and ext-webNavigation.js [2] should be removed. It would also be a good idea to audit the use of ignoreEvent throughout the other APIs [3] to confirm whether they should be removed or not. [1] http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-tabs.js#346 [2] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-webNavigation.js#164 [3] http://searchfox.org/mozilla-central/search?q=ignoreEvent(&case=false®exp=false&path=
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(adam.schenker)
Priority: -- → P3
Summary: chrome.tabs.onReplaced.addListener unimplemented → Remove ignoreEvent from API events for which it is inappropriate
Reporter | ||
Comment 8•8 years ago
|
||
I would also suggest updating the API documentation (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onReplaced) to reflect that these functions currently won't do anything. A brief explanation of the technical basis may also be helpful. Is it preferred that extension developers remove code using this API (as opposed to leaving it in and ignoring the warning)?
Comment 9•8 years ago
|
||
Updating the documentation is a good suggestion, and something you could actually do yourself if you have the time. MDN is a wiki that is open to anyone for editing. I wouldn't say that you should remove the code from your extension, the reason the event is implemented but ignored is to allow an extension to be coded for both Firefox and Chrome. The warning is just a warning, and so can be safely ignored. When this bug is fixed it will go away.
Updated•8 years ago
|
Whiteboard: triaged
Updated•8 years ago
|
Whiteboard: triaged → triaged[good first bug]
Comment 10•7 years ago
|
||
If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started.
Mentor: bob.silverberg
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Hi Bob, I have removed ignoreEvent from some APIs where it seems unnecessary. Please have a look and let me know if any further changes are required. Thanks!! Regards
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8841245 [details] Bug 1305528 - Remove ignoreEvent from API events for which it is inappropriate. https://reviewboard.mozilla.org/r/115522/#review117104 Thank you for the patch, Tushar, this is really nice work. The changes you have made to remove ignoreEvent from tabs.onReplaced and webNavigation.onTabReplaced look good. As I mentioned in the bug, it would be good to also take a look at any other places in our code where we are using ignoreEvent to make sure that they are still appropriate. I did a quick search and I found we are also using ignoreEvent for downloads.onDeterminingFilename and notifications.onButtonClicked. The latter is definitely okay, as we may implement notifications.onButtonClicked at some point, but I am not sure about downloads.onDeterminingFilename. I also noticed that although we have an API event defined for downloads.onDeterminingFilename in ext-downloads.js, the event is not defined in the schema file (downloads.json), which means the ignoreEvent call in ext-downloads.js will never fire, because an exception will be thrown by our schema validation code if a developer tries to call browser.downloads.onDeterminingFilename.addListener() etc. This should probably be fixed, and I think it makes sense to do it as part of this bug. I think the solution to that issue is to either: 1. Remove the code for downloads.onDeterminingFilename from ext-downloads.js, if we are happy to have an exception thrown when a developer attempts to use the downloads.onDeterminingFilename event. 2. Add a definition for downloads.onDeterminingFilename to the downloads.json file, which will result in a warning being issued when a developer attempts to use the downloads.onDeterminingFilename event, which would have been the intent of the ignoreEvent in the first place. I’m going to add a needinfo for Andrew Swan who is the most knowledgable about this API to let us know which of the above routes should be taken, or if something different from either of those should be done about downloads.onDeterminingFilename. Once we have an answer on that you can make whatever changes are necessary and then push your changes to MozReview and request another review.
Attachment #8841245 -
Flags: review?(bob.silverberg)
Comment 14•7 years ago
|
||
Andrew, can you please take a look at the above comments about downloads.onDeterminingFilename and let us know what you think should be done about that?
Flags: needinfo?(aswan)
Updated•7 years ago
|
Assignee: nobody → tushar.saini1285
Status: NEW → ASSIGNED
Comment 15•7 years ago
|
||
I'm not sure why onDeterminingFilename was omitted from the schema but we have an open bug to (eventually) implement it so it should be added to the schema, though that may be outside the scope of this bug. Also, I took a look at the patch and I don't think its correct, events aren't functions, they are objects with addListener(), removeListener(), and hasListener() functions.
Flags: needinfo?(aswan)
Comment 16•7 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #15) > I'm not sure why onDeterminingFilename was omitted from the schema but we > have an open bug to (eventually) implement it so it should be added to the > schema, though that may be outside the scope of this bug. > Also, I took a look at the patch and I don't think its correct, events > aren't functions, they are objects with addListener(), removeListener(), and > hasListener() functions. Thanks Andrew. It sounds like we can just ignore (pardon the pun) onDeterminingFilename for the purposes of this bug. Tushar, Andrew is right that your current code does not do what it intends to do. What we need to do, instead of this, is update the ignoreEvent function in ExtensionUtils.jsm to add an argument that will control whether or not a warning is issued. We can then continue to use ignoreEvent in tabs.onReplaced and webNavigation.onTabReplaced, but pass into it an argument to say we do not want a warning. This should still be a fairly easy thing to fix, so I think you can tackle it via this bug. [1] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionUtils.jsm#700
Comment 17•7 years ago
|
||
I stand corrected, yet again. We should not change ignoreEvent. Instead, in the places where we shouldn't be using ignoreEvent, we can just use a no-op SingletonEventManager. So it would look similar to the other events in the file that are implemented, but without any specific implementation.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•7 years ago
|
||
Hey, Thanks for the help. I have modified the patch based on your inputs, please have a look. Regards.
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8841245 [details] Bug 1305528 - Remove ignoreEvent from API events for which it is inappropriate. https://reviewboard.mozilla.org/r/115522/#review117972 Nice work, Tushar! This looks really good except for a few minor comments that I have added below. Please address those and then submit for review again. ::: browser/components/extensions/ext-tabs.js:216 (Diff revision 2) > return () => { > tabTracker.off("tab-removed", listener); > }; > }).api(), > > - onReplaced: ignoreEvent(context, "tabs.onReplaced"), > + onReplaced: new SingletonEventManager(context, "tabs.onAttached", fire => { `tabs.onAttached` should be `tabs.onReplaced` ::: mobile/android/components/extensions/ext-tabs.js:206 (Diff revision 2) > return () => { > tabTracker.off("tab-removed", listener); > }; > }).api(), > > - onReplaced: ignoreEvent(context, "tabs.onReplaced"), > + onReplaced: new SingletonEventManager(context, "tabs.onAttached", fire => { `tabs.onAttached` should be `tabs.onReplaced` ::: toolkit/components/extensions/ext-downloads.js (Diff revision 2) > XPCOMUtils.defineLazyModuleGetter(this, "EventEmitter", > "resource://devtools/shared/event-emitter.js"); > > Cu.import("resource://gre/modules/ExtensionUtils.jsm"); > const { > - ignoreEvent, You don't need to make any modifications to this file. As mentioned in an earlier comment, there are other issues here, but they will be dealt with via a different bug. ::: toolkit/components/extensions/ext-downloads.js:794 (Diff revision 2) > - onDeterminingFilename: ignoreEvent(context, "downloads.onDeterminingFilename"), > + onDeterminingFilename: new SingletonEventManager(context, "tabs.onAttached", fire => { > + return () => {}; > + }).api(), As above, please revert these changes. ::: toolkit/components/extensions/ext-webNavigation.js:160 (Diff revision 2) > extensions.registerSchemaAPI("webNavigation", "addon_parent", context => { > let {tabManager} = context.extension; > > return { > webNavigation: { > - onTabReplaced: ignoreEvent(context, "webNavigation.onTabReplaced"), > + onTabReplaced: new SingletonEventManager(context, "tabs.onAttached", fire => { `tabs.onAttached` should be `webNavigation.onTabReplaced` ::: toolkit/components/extensions/ext-webNavigation.js:170 (Diff revision 2) > onDOMContentLoaded: new WebNavigationEventManager(context, "onDOMContentLoaded").api(), > onCompleted: new WebNavigationEventManager(context, "onCompleted").api(), > onErrorOccurred: new WebNavigationEventManager(context, "onErrorOccurred").api(), > onReferenceFragmentUpdated: new WebNavigationEventManager(context, "onReferenceFragmentUpdated").api(), > onHistoryStateUpdated: new WebNavigationEventManager(context, "onHistoryStateUpdated").api(), > - onCreatedNavigationTarget: ignoreEvent(context, "webNavigation.onCreatedNavigationTarget"), > + onCreatedNavigationTarget: new SingletonEventManager(context, "tabs.onAttached", fire => { The `ignoreEvent` should remain on this event, as this is something that will be implemented in the future (and in fact is in development right now).
Attachment #8841245 -
Flags: review?(bob.silverberg)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
Hey, sorry for the rookie mistake :$. I will be more carefull from now on. And I think this will be ready to land. Thanks for all your help!!
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8841245 [details] Bug 1305528 - Remove ignoreEvent from API events for which it is inappropriate. https://reviewboard.mozilla.org/r/115522/#review117984 Thanks Tushar. Great work! This looks good to me, but it needs an r+ from a peer to land. I'm going to request that on your behalf.
Attachment #8841245 -
Flags: review?(bob.silverberg) → review+
Comment 24•7 years ago
|
||
Comment on attachment 8841245 [details] Bug 1305528 - Remove ignoreEvent from API events for which it is inappropriate. Shane, can you please give this a look when you have a chance?
Attachment #8841245 -
Flags: review?(mixedpuppy)
Comment 25•7 years ago
|
||
We need to update the docs on this, it does look like this is supported functionality from reading mdn. Maybe we need a followup bug, looks like other items are like this (e.g. tabs.onMoved on android)
Keywords: dev-doc-needed
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8841245 [details] Bug 1305528 - Remove ignoreEvent from API events for which it is inappropriate. https://reviewboard.mozilla.org/r/115522/#review118462
Attachment #8841245 -
Flags: review?(mixedpuppy) → review+
Comment 27•7 years ago
|
||
Pushed by bsilverberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/37683d1886aa Remove ignoreEvent from API events for which it is inappropriate. r=bsilverberg,mixedpuppy
Comment 28•7 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/c6506764b461 Remove ignoreEvent from API events for which it is inappropriate: remove unused ignoreEvent from ext-webNavigation.js. r=eslint-fix
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/37683d1886aa https://hg.mozilla.org/mozilla-central/rev/c6506764b461
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 30•7 years ago
|
||
Nice work, Tushar. This has landed in Firefox Nightly.
Comment 31•7 years ago
|
||
I've added notes for: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/onReplaced#Browser_compatibility https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webNavigation/onTabReplaced#Browser_compatibility ... to say that although you can add listeners for these events, they will never fire. Does that cover it for this bug? I wasn't quite sure if anything else was needed.
Flags: needinfo?(bob.silverberg)
Comment 32•7 years ago
|
||
Looks good, Will. Thanks for taking care of that.
Flags: needinfo?(bob.silverberg)
Updated•7 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•