Closed Bug 1305528 Opened 3 years ago Closed 3 years ago

Remove ignoreEvent from API events for which it is inappropriate

Categories

(WebExtensions :: General, defect, P3)

x86_64
macOS
defect

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)

Attached file TestExtension.xpi
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
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
The upload uses browser.tabs.onReplaced.addListener but the same behavior happens with chrome.tabs.onReplaced.addListener.
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
(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.
I can't reproduce either in Firefox 49, OS X 10.10.5.
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.
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.
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&regexp=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
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)?
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.
Whiteboard: triaged
Whiteboard: triaged → triaged[good first bug]
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
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 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)
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)
Assignee: nobody → tushar.saini1285
Status: NEW → ASSIGNED
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)
(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
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.
Hey,

 Thanks for the help. I have modified the patch based on your inputs, please have a look.

Regards.
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)
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 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 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)
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 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+
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
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
https://hg.mozilla.org/mozilla-central/rev/37683d1886aa
https://hg.mozilla.org/mozilla-central/rev/c6506764b461
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Nice work, Tushar. This has landed in Firefox Nightly.
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)
Looks good, Will. Thanks for taking care of that.
Flags: needinfo?(bob.silverberg)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.