Closed Bug 1300584 Opened 8 years ago Closed 7 years ago

Implements devtools.inspectedWindow.eval and reload API methods

Categories

(WebExtensions :: Developer Tools, defect, P1)

Product:
WebExtensions
Component:
Developer Tools
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: rpl, Assigned: rpl)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [devtools][devtools.inspectedWindow] triaged)

Attachments

(2 files, 2 obsolete files)

Bug 1300584 - Implements devtools.inspectedWindow.eval.
58 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1300584 - Implements devtools.inspectedWindow.reload.
58 bytes, text/x-review-board-request
kmag
: review+
Details 
The goal of this issue is to implement, on top of the minimal subset of devtools API implementation introduced by Bug 1291737, the devtools.inspectedWindow eval and reload API methods.

eval and reload are the first two devtools API methods to actually need to use the Remote Debugging Protocol (the inspectedWindow.tabId getter only needs to retrieve the current target tab element to resolve its WebExtension tabId).

To communicate with the Remote Debugging Server, the WebExtension devtools API should use a different connection from the one used by the integrated developer tools (the main reason is to minimize the negative impact of issues related to an addon devtools_page/devtools_panel on the integrated tools, which is similar to what the current Addon SDK bindings does).

To be able to create a TabTarget that does not reuse the existent debugging connection to the target tab, the TabTarget class have to be exported from the devtools module that declares it.

Another goal of this issue is the creation of a new actor (which will provide its own spec and front), mainly because the eval and reload API methods have some WebExtension specific features that are not needed nor available in the current Remote Debugging Server actors (e.g. support this WebExtension specific options, e.g. the eval method should report differently an exception happened in the evaluated code and an error happened in the WebExtensions or the Devtools API code, and the reload method supports an injectedScript options), and even more WebExtension specific features will be implemented in follow ups.
Blocks: 1211859
Depends on: 1291737
Priority: -- → P1
Whiteboard: [devtools][devtools.inspectedWindow] triaged
Blocks: 1300587
Blocks: 1300588
Blocks: 1300590
Comment on attachment 8788244 [details]
Bug 1300584 - Implements devtools.inspectedWindow.reload.

https://reviewboard.mozilla.org/r/76812/#review76998

::: browser/components/extensions/ext-devtools.js:29
(Diff revision 1)
> +let devtoolsTargetForContext = new Map();
> +global.getLocalTabTargetForContext = function(context, target) {
> +  const {TabTarget} = require("devtools/client/framework/target");
> +
> +  return Task.spawn(function* asyncConnectLocalTabTarget() {
> +    if (devtoolsTargetForContext.has(context)) {

nit: If you check for `!devtoolsTargetForContext.has(context)` instead, and then do the initialization of the `newTarget` if needed, you could simply return `devtoolsTargetForContext.get(context)` at the end and not require two `return` statements.

::: browser/components/extensions/schemas/devtools_inspected_window.json
(Diff revision 1)
>        {
>          "name": "eval",
> -        "unsupported": true,
>          "type": "function",
>          "description": "Evaluates a JavaScript expression in the context of the main frame of the inspected page. The expression must evaluate to a JSON-compliant object, otherwise an exception is thrown. The eval function can report either a DevTools-side error or a JavaScript exception that occurs during evaluation. In either case, the <code>result</code> parameter of the callback is <code>undefined</code>. In the case of a DevTools-side error, the <code>isException</code> parameter is non-null and has <code>isError</code> set to true and <code>code</code> set to an error code. In the case of a JavaScript error, <code>isException</code> is set to true and <code>value</code> is set to the string value of thrown object.",
> -        "async": "callback",

This is synchronous?
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

Hi Alex,
As briefly discussed on IRC, this patch contains a new actor (and its related spec and front) for the Remote Debugging Server which I'm using as backend for the WebExtensions DevTools API that I'm working on (e.g. as it is currently used in the proposed patch attached on this same issue as attachment 8788244 [details])

I've currently prepared an alternative version of the WebExtensions DevTools API patches, which are functionally the same of the patches current attached to this and its related issues, but with the changes needed to work in an "webext-oop" world (Bug 1190679), and the changes related to webext-oop are not affecting the patches that are related to the Firefox DevTools internals, and so we can start to take a look at the actor (while the webext-oop changes are going to be discussed separately), to be sure that it follows correctly the coding style and conventions that a new RDP actor should follow.
Attachment #8788242 - Flags: feedback?(poirot.alex)
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76806/#review83790

::: browser/components/extensions/ext-devtools-inspectedWindow.js:72
(Diff revision 4)
> +            // TODO(rpl): check for additional undocumented behaviors on chrome
> +            // (e.g. if we should also print error to the console or set lastError?).
> +            return Promise.resolve(new SpreadArgs([res.evalResult, evalErrorOrException]));
> +          };
> +
> +          return getLocalTabTargetForContext(context, toolbox.target).then(clonedTarget => {

You also get a target object from the toolbox object, I don't get why you have to create another one??
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76808/#review83780

::: devtools/server/actors/webextension-devtools-api.js:57
(Diff revision 3)
> +      }
> +    },
> +
> +    reload(ignoreCache, {userAgent, injectedScript}) {
> +      const dispatchInfallible = (desc, cb) =>
> +        Services.tm.currentThread.dispatch(DevToolsUtils.makeInfallible(cb, desc), 0);

makeInfaillible is only useful to:
1/ prevent stopping a code when some exception happens
2/ ensure logging the exception if one happens
Platform evolved, now the exception are correctly printed, so you should check if that is necessary at all.

Also do you really have to run this on the next event loop cycle?

::: devtools/server/actors/webextension-devtools-api.js:87
(Diff revision 3)
> +          };
> +
> +          events.once(this.tabActor, "window-ready", () => {
> +            // Wait for the fully loaded page.
> +            this.window.addEventListener("load", loadedListener);
> +          });

It's hard to get why you are listening for all these things. Why are you listening for window-ready?
Why aren't you removing DOMContentLoaded from injectScriptListener?

::: devtools/server/actors/webextension-devtools-api.js:174
(Diff revision 3)
> +            details: [
> +              String(err),
> +            ],
> +          };
> +        }
> +      }

So, at the end what are the precise differences with the webconsole eval method?
Could we at least share some code? You may also say that's not worth it. I'm just asking here.

::: devtools/server/main.js:579
(Diff revision 3)
>        constructor: "EmulationActor",
>        type: { tab: true }
>      });
> +    this.registerModule("devtools/server/actors/webextension-devtools-api", {
> +      prefix: "webExtensionInspectedWindow",
> +      constructor: "WebExtensionInspectedWindowActor",

File name and constructor should better match.
Do you think this actor is going to implemente only apis useful for InspectorWindow api or is it going to be augmented to help implement some otherts?
Attachment #8788242 - Flags: feedback?(poirot.alex)
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76808/#review83780

> makeInfaillible is only useful to:
> 1/ prevent stopping a code when some exception happens
> 2/ ensure logging the exception if one happens
> Platform evolved, now the exception are correctly printed, so you should check if that is necessary at all.
> 
> Also do you really have to run this on the next event loop cycle?

Sounds good, I took inspiration from the reload method of the TabActor, maybe it was necessary at the time but it is not anymore.

+1 I'm going to expand the tests and check if makeInfallible is really usefull.

Executing it on the next event loop cycle is something that I was doing so that the response packet generated by the return can reach the client before the packet before the navigation would actually happen.

> It's hard to get why you are listening for all these things. Why are you listening for window-ready?
> Why aren't you removing DOMContentLoaded from injectScriptListener?

Yeah, I agree, this is definitely the less clear part of this actor, and I'm not very happy of the current implementation :-(

The goal is to support the `injectedScript` parameter: 
- if specified, it has to be injected in every frame created in the page during its upcoming reload;
- to achieve this I'm currently listening to `"DOMContentLoaded"` events (to inject the script in every frame created during the page load);
- on the next `"window-ready"` event received, the `"load"` event is subscribed;
- once the `"load"` event has been received, the `"DOMContentLoaded"` event listener can be unsubscribed because the page is fully loaded.

I'm going to create some tests for it, and then I'm going to look into its implementation again (further advices and suggestions in this area are absolutely welcome ;-))

> So, at the end what are the precise differences with the webconsole eval method?
> Could we at least share some code? You may also say that's not worth it. I'm just asking here.

Sure, that's a good question, and I'm still keeping an eye on how much of the two `eval` can be shared.

Currently, most of the needs of the eval are accomplished by `dbgWindow.executeInGlobalWithBindings`, and so it is currently shared by both.

The post processing of the result is pretty similar in both the actors, but the `inspectedWindow`'s `eval` is different in what is returned to the client: it has to return a json representation of the result (and not the object grip), if the result cannot be serialized, it has to be reported as a "protocol error" (on the contrary, if the result cannot be returned for an exception in the evaluated code, it has to be reported as an exception).

Nevertheless, in follow-ups they can probably share more, e.g. some of the `bindings` provided during the evaluation (e.g. `$0` and `inspect`), but some of them should probably be omitted from the `inspectedWindow.eval` (e.g. `$_` which gives access to the last evaluation result`).

Additonal side-notes: the `inspetedWindow.eval` will probably diverge even more in terms of provided feature, e.g. once we are going to look into implementing the other options (e.g. `frameURL`, which requires that the expression is evaluated in frames with the defined url, and `useContentScriptContext`, which requires that the expression is evaluated in the contentScript of the calling extensions, instead of the page global).

> File name and constructor should better match.
> Do you think this actor is going to implemente only apis useful for InspectorWindow api or is it going to be augmented to help implement some otherts?

Yep, I would like to keep the single actors as simple as possible, and in my current plan this actor is the "backend" of the `devtools.inspectedWindow` API (the `devtools.network` API currently doesn't need its own actor, not yet at least, but once we need an actor for it, the plan is to create a separate actor as its backend).

Anyway, I completely agree, I'm going to rename this file into `"devtools/server/actor/webextension-inspected-window.js"` (and the other ones would be `"devtools/server/actor/webextension-network.js"` etc.)

how it sounds to you?
Thanks Alex, as usual, for your thoughtful and detailed feedback.

I'm going to apply most of this suggestions immediately (and take a deeper look into the rest of them).
Assignee: nobody → lgreco
(In reply to Luca Greco [:rpl] from comment #19)
> Comment on attachment 8788242 [details]
> I'm going to create some tests for it, and then I'm going to look into its
> implementation again (further advices and suggestions in this area are
> absolutely welcome ;-))

I'm wondering if that would be any heplful to reuse existing code from WebExtension?
I imagine you are also listening for all these load events... but may be that's easier to reuse devtools events from here?
 
> Additonal side-notes: the `inspetedWindow.eval` will probably diverge even
> more in terms of provided feature, e.g. once we are going to look into
> implementing the other options (e.g. `frameURL`, which requires that the
> expression is evaluated in frames with the defined url, and
> `useContentScriptContext`, which requires that the expression is evaluated
> in the contentScript of the calling extensions, instead of the page global).

Same here, I'm wondering...
Isn't there some code in webextension to inject a script only in specific frames?

> Anyway, I completely agree, I'm going to rename this file into
> `"devtools/server/actor/webextension-inspected-window.js"` (and the other
> ones would be `"devtools/server/actor/webextension-network.js"` etc.)
> 
> how it sounds to you?

Good ;)
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76808/#review83780

> Sounds good, I took inspiration from the reload method of the TabActor, maybe it was necessary at the time but it is not anymore.
> 
> +1 I'm going to expand the tests and check if makeInfallible is really usefull.
> 
> Executing it on the next event loop cycle is something that I was doing so that the response packet generated by the return can reach the client before the packet before the navigation would actually happen.

I tried to remove makeInfallible from the new reload event and it doesn't seem to add anything on the plate (as you pointed out in the previous feedback comments), an exception error message seems to be reported correctly even without the makeInfallible wrapping.

And so makeInfallible has been removed from the updated version of this actor method.

But I'm not clearing this mozreview issue because I'm still running the reload on the next event loop cycle, 
is it not necessary to achieve what I described in the above comment?

> Yeah, I agree, this is definitely the less clear part of this actor, and I'm not very happy of the current implementation :-(
> 
> The goal is to support the `injectedScript` parameter: 
> - if specified, it has to be injected in every frame created in the page during its upcoming reload;
> - to achieve this I'm currently listening to `"DOMContentLoaded"` events (to inject the script in every frame created during the page load);
> - on the next `"window-ready"` event received, the `"load"` event is subscribed;
> - once the `"load"` event has been received, the `"DOMContentLoaded"` event listener can be unsubscribed because the page is fully loaded.
> 
> I'm going to create some tests for it, and then I'm going to look into its implementation again (further advices and suggestions in this area are absolutely welcome ;-))

I've reworked most of the reload method and added tests for it.

The new approach is composed of:

- subscribe the actor as an observer of 'document-element-inserted' (mostly like we track the loading of webpage frames in ExtensionContent.jsm to inject the content script)
- explicitly prevent two "customized" reloads (a reload with a custom user agent or an injected script) to happen at the same time
- inject the script when the page is loading (before any other webpage script has been yet loaded, as described in the API method docs) if the window is the top level frame of the target tab, or if it's a child of an already injected window (basically any descendent frame that has been created during the page load)
- listen for the "load" event of the new top level frame, once the top level frame has been loaded, clear the current customized reload (unsubscribe the events and clear the related actor properties) 
- explicitly prevent a customized reload on a SystemPrincipal target (e.g. about:addons)
- clear any pending customized reload (and the related subscribed events and actor properties) when the actor has been destoyed

The actor is curently subscribed to the 'document-element-inserted' only if there is a pending customized reload (and unsubscribed when the reload has been completed or the actor destroyed).

> Yep, I would like to keep the single actors as simple as possible, and in my current plan this actor is the "backend" of the `devtools.inspectedWindow` API (the `devtools.network` API currently doesn't need its own actor, not yet at least, but once we need an actor for it, the plan is to create a separate actor as its backend).
> 
> Anyway, I completely agree, I'm going to rename this file into `"devtools/server/actor/webextension-inspected-window.js"` (and the other ones would be `"devtools/server/actor/webextension-network.js"` etc.)
> 
> how it sounds to you?

This actor file name has been changed into "webextension-inspected-window.js" (and the other related files, front, spec and test file have been renamed accordingly).
(In reply to Alexandre Poirot [:ochameau] from comment #21)
> (In reply to Luca Greco [:rpl] from comment #19)
> > Comment on attachment 8788242 [details]
> > I'm going to create some tests for it, and then I'm going to look into its
> > implementation again (further advices and suggestions in this area are
> > absolutely welcome ;-))
> 
> I'm wondering if that would be any heplful to reuse existing code from
> WebExtension?
> I imagine you are also listening for all these load events... but may be
> that's easier to reuse devtools events from here?

A took a second look at the ExtensionContent.jsm to plan the new reworked reload method of this actor and it seems to me that reload API method needs are much more simpler and there could be enough differences to keep them into two separate implementations (if it can be achieved without actually duplicating the exact same code), e.g.:

- in ExtensionContent.jsm the javascript code is currently always injected in the content script sandbox, while devtools.inspectedWindow.reload is always injected in the content window (and always executed before any webpage javascript code)
- the devtools.inspectedWindow.reload should be able to customize the userAgent during the page reload, while the ExtensionContent.jsm content scripts do not need this feature.

> > Additonal side-notes: the `inspetedWindow.eval` will probably diverge even
> > more in terms of provided feature, e.g. once we are going to look into
> > implementing the other options (e.g. `frameURL`, which requires that the
> > expression is evaluated in frames with the defined url, and
> > `useContentScriptContext`, which requires that the expression is evaluated
> > in the contentScript of the calling extensions, instead of the page global).
> 

> Same here, I'm wondering...
> Isn't there some code in webextension to inject a script only in specific frames?

In follow ups, this actor is going to be integrate with ExtensionContent.jsm for sure (e.g. in follow ups related to the 'useContentScriptContext' eval method parameter).

For the evaluation of javascript code in a specific frameURL or in the content script sandbox, I'd like if possible to use `executeInGlobalWithBindings`, because (if I'm not wrong) it guarantees that the evaluation will work even if the debugger is currently paused on a breakpoint (which is something that a devtools addon should be probably able to do).

Additional side note: I'm also trying to always pass through this actor for all the API methods that are related to the "remote target" (even if we are not going to support remote target immediately, I'm trying to keep the implementation as "remote target"-friendly as possible).

> 
> > Anyway, I completely agree, I'm going to rename this file into
> > `"devtools/server/actor/webextension-inspected-window.js"` (and the other
> > ones would be `"devtools/server/actor/webextension-network.js"` etc.)
> > 
> > how it sounds to you?
> 
> Good ;)

Great!

I've updated the actor patch on this bugzilla issue, so that its inter-diff can be simpler to read, but I'd like to move the actor patch into a separate bugzilla issue so that we can review it and land it separately from the changes on the webextensions internals.

Do you mind if I'm going to file the new bugzilla issue and move the patch with the actor to a new mozreview request attached to it?
Flags: needinfo?(poirot.alex)
Status: NEW → ASSIGNED
(In reply to Luca Greco [:rpl] from comment #27)
> Do you mind if I'm going to file the new bugzilla issue and move the patch
> with the actor to a new mozreview request attached to it?

Please do.
Flags: needinfo?(poirot.alex)
(In reply to Luca Greco [:rpl] from comment #26)
> But I'm not clearing this mozreview issue because I'm still running the
> reload on the next event loop cycle, 
> is it not necessary to achieve what I described in the above comment?

If reload happens to take a while, it allows to release RDP and exchange other messages.
So it can be a good thing to resolve immediately if the code you are calling is blocking synchronously for a while.
I think nsIWebNavigation.reload() doesn't block until the reload is done, but it may block if there is some onbeforeunload prompts...
(In reply to Luca Greco [:rpl] from comment #27)
> (In reply to Alexandre Poirot [:ochameau] from comment #21)
> > (In reply to Luca Greco [:rpl] from comment #19)
> > > Comment on attachment 8788242 [details]
> > > I'm going to create some tests for it, and then I'm going to look into its
> > > implementation again (further advices and suggestions in this area are
> > > absolutely welcome ;-))
> > 
> > I'm wondering if that would be any heplful to reuse existing code from
> > WebExtension?
> > I imagine you are also listening for all these load events... but may be
> > that's easier to reuse devtools events from here?
> 
> A took a second look at the ExtensionContent.jsm to plan the new reworked
> reload method of this actor and it seems to me that reload API method needs
> are much more simpler and there could be enough differences to keep them
> into two separate implementations (if it can be achieved without actually
> duplicating the exact same code), e.g.:
> 
> - in ExtensionContent.jsm the javascript code is currently always injected
> in the content script sandbox, while devtools.inspectedWindow.reload is
> always injected in the content window (and always executed before any
> webpage javascript code)
> - the devtools.inspectedWindow.reload should be able to customize the
> userAgent during the page reload, while the ExtensionContent.jsm content
> scripts do not need this feature.
> 
> > > Additonal side-notes: the `inspetedWindow.eval` will probably diverge even
> > > more in terms of provided feature, e.g. once we are going to look into
> > > implementing the other options (e.g. `frameURL`, which requires that the
> > > expression is evaluated in frames with the defined url, and
> > > `useContentScriptContext`, which requires that the expression is evaluated
> > > in the contentScript of the calling extensions, instead of the page global).
> > 
> 
> > Same here, I'm wondering...
> > Isn't there some code in webextension to inject a script only in specific frames?
> 
> In follow ups, this actor is going to be integrate with ExtensionContent.jsm
> for sure (e.g. in follow ups related to the 'useContentScriptContext' eval
> method parameter).
> 
> For the evaluation of javascript code in a specific frameURL or in the
> content script sandbox, I'd like if possible to use
> `executeInGlobalWithBindings`, because (if I'm not wrong) it guarantees that
> the evaluation will work even if the debugger is currently paused on a
> breakpoint (which is something that a devtools addon should be probably able
> to do).

It's not just being able to eval while the debugger is paused. (It is not specific in google docs)
https://developer.chrome.com/extensions/devtools_inspectedWindow#executing-code
But more about having the exact same scope as the web console, with all "$" globals!

> Additional side note: I'm also trying to always pass through this actor for
> all the API methods that are related to the "remote target" (even if we are
> not going to support remote target immediately, I'm trying to keep the
> implementation as "remote target"-friendly as possible).

Yes, I imagine you may also implement everything using the E10S abstractions already used by web extension codebase (frame scripts and message manager?)...
I imagine you could have evalInGlobalWithBinding to work even without any actor.
It may use less memory/cpu than using devtools and its actors. I'm not sure there is any value in having web extension APIs working with real remote targets like fennec?
(In reply to Alexandre Poirot [:ochameau] from comment #30)
> It's not just being able to eval while the debugger is paused. (It is not
> specific in google docs)
> https://developer.chrome.com/extensions/devtools_inspectedWindow#executing-
> code
> But more about having the exact same scope as the web console, with all "$"
> globals!

Absolutely, the main point is that the inspectedWindow.eval is supposed to work more like the webconsole evaluation (which is the opposite of what is the supposed behavior of any other webextension APIs related to javascript code evaluation), and it is supposed to have access to (at least a subset) of the helpers available in the webconsole (e.g. `inspect(...)` and `$0` is the most common used)

(as a side note: I'm not currently sure that the `reload` method's injectedScripts have this bindings available on Chrome, their documentation seems to always refer to the eval method)

> Yes, I imagine you may also implement everything using the E10S abstractions
> already used by web extension codebase (frame scripts and message
> manager?)...
> I imagine you could have evalInGlobalWithBinding to work even without any
> actor.

Yes, absolutely: it is definitely possible to achieve it without using any actor (e.g. in one of the initial prototypes, I tried different approaches for the eval method to check which latency everyone of them was introducing)

An additional reason that is pushing me to use a devtools actor as the preferred implementation strategy for these inspectedWindow API is that
a lot of the feature that are provided by these APIs are already implemented in the available devtools actors (e.g. the devtools.network can be probably be implemented by using the existent actors methods and events), and so it would make the various WebExtensions devtools APIs implementations more uniform.

> It may use less memory/cpu than using devtools and its actors. I'm not sure
> there is any value in having web extension APIs working with real remote
> targets like fennec?

That's true, but given that this API are only available in contexts that are created when a devtools toolbox is opened, the devtools and its actors (besides this one, specifically used by the devtools.inspectedWindow API) will be loaded in any case, and so a large amount of that additional memory/cpu would be allocated in any case, am I wrong?

Besides that, in the long run, being able to use a devtools addon when the toolbox is connected to a Firefox for Android remote target is probably a nice feature (and it was something that devtools addons have already requested in the past, e.g. the Ember Inspector), and it would be probably much more nicer if it can work with the same APIs.

Anyway, I'm open to rethink it if we end up to conclude that there are enough reasons to do it without an actor.

In the meantime, I'm going to open a separate issue, so that we can continue our discussion and evaluations there.

I'm very happy that we are discussing and dissecting everyone of these details, re-evaluating "the picture" with a second pair of eyes and being more sure that important details are not being overlooked .
I'm giving a beer to the dev that implements this. 5$ on Bountysource :) I know it's not much but I can't wait for Chrome DevTools extensions to work in Firefox also :) I might increase the amount in the future.
Depends on: 1315251
Attachment #8788245 - Attachment is obsolete: true
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76808/#review90810

::: browser/components/extensions/ext-devtools-inspectedWindow.js:23
(Diff revision 5)
> +  let inspectedWindowFront;
> +  function getInspectedWindowFront() {
> +    return DevtoolsContextsManager
> +      .getLocalTabTargetForContext(context)
> +      .then(clonedTarget => {
> +        if (!inspectedWindowFront) {

It looks like we're only going to have to create a new `WebExtensionInspectedWindowFront` if `inspectedWindowFront` is undefined, so why not move this check to line 20, and not even bother calling `DevtoolsContextsManager.getLocalTabTargetForContext` unless `inspectedWindowFront` is undefined?

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:131
(Diff revision 5)
> +          });
> +        })
> +        .catch(err => browser.test.notifyFail(`Unexpected error in test: ${err.message}`));
> +    }
> +
> +    function testCallbackEval(args) {

Just wondering why you are explicitly testing the API when used with a callback. We don't generally do that. I'm not suggesting it's a bad thing, I actually think it's a good idea, but was wondering if there is something specific about devtools APIs or this `eval` method that made you decide it was important to test both the promise and callback forms?
Attachment #8788243 - Attachment is obsolete: true
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76808/#review90810

> It looks like we're only going to have to create a new `WebExtensionInspectedWindowFront` if `inspectedWindowFront` is undefined, so why not move this check to line 20, and not even bother calling `DevtoolsContextsManager.getLocalTabTargetForContext` unless `inspectedWindowFront` is undefined?

That's true, I changed this helper as suggested.

> Just wondering why you are explicitly testing the API when used with a callback. We don't generally do that. I'm not suggesting it's a bad thing, I actually think it's a good idea, but was wondering if there is something specific about devtools APIs or this `eval` method that made you decide it was important to test both the promise and callback forms?

Thanks for pointing it out, I double-checked if it was still needed and I noticed that it is not needed anymore. 

The test was checking the two modes (callback and promised) explicitly because in a previous version there was an eval method on the client side to check if the API call has received a callback or not, and then call the callback with two parameters or return a promise which resolves to the array of parameters.

The reason why it was needed it is definitely related to the webext-oop changes not handling correctly SpreadArgs, but it has been fixed recently on mozilla-central and so I've removed both the client side part of the inspectedWindow.eval method, and now I've removed the explicit tests of the callback mode.
webextensions: --- → +
Attachment #8788242 - Flags: review?(kmaglione+bmo)
Attachment #8788244 - Flags: review?(kmaglione+bmo)
No longer blocks: 1300587
No longer blocks: 1300588
Comment on attachment 8788244 [details]
Bug 1300584 - Implements devtools.inspectedWindow.reload.

https://reviewboard.mozilla.org/r/76812/#review107172

r=me with issues addressed.

::: browser/components/extensions/ext-devtools-inspectedWindow.js:58
(Diff revision 12)
> +          getInspectedWindowFront().then(inspectedWindowFront => {
> +            inspectedWindowFront.reload(callerInfo, {ignoreCache, userAgent, injectedScript});
> +          });

s/inspectedWindowFront/win/g

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_reload.js:33
(Diff revision 12)
> +      <html>
> +       <head>
> +         <meta charset="utf-8">
> +       </head>
> +       <body>
> +         <script src="devtools_page.js"></script>

type="text/javascript"

And please move this to the <head>

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_reload.js:96
(Diff revision 12)
> +          // This test expects two `devtools.inspectedWindow.reload` calls:
> +          // the first one without any options and the second one with
> +          // `ignoreCache=true`.
> +          switch (reloads) {
> +            case 1:
> +              browser.tabs.executeScript({code: `(${checkIgnoreCache})(false)`});

Please replace this with something like:

    let matches = await executeScript(tabId, {code: `document.body.textContent.includes("empty cache headers")`});
    browser.test.assertTrue(...);

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_reload.js:168
(Diff revision 12)
> +        if (details.tabId == activeTabId) {
> +          reloads++;
> +
> +          switch (reloads) {
> +            case 1:
> +              browser.tabs.executeScript({code: `(${checkCustomUserAgent})(false)`});

await executeScript(...);

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_reload.js:217
(Diff revision 12)
> +    function checkInjectedScript(enabled) {
> +      let results = [];
> +      let iframeDoc = document;
> +
> +      while (iframeDoc) {
> +        results.push(iframeDoc.querySelector("pre").textContent);
> +        const iframe = iframeDoc.querySelector("iframe");
> +        iframeDoc = iframe ? iframe.contentDocument : null;
> +      }
> +
> +      let expectedResults = (new Array(4)).fill(
> +        enabled ? "injected script executed first" : "injected script NOT executed"
> +      );
> +
> +      browser.test.assertEq(JSON.stringify(expectedResults), JSON.stringify(results),
> +                            `Got the expected result with injectScript=${enabled}`);
> +      browser.test.sendMessage(
> +        `devtools_inspectedWindow_reload_injectedScript.done`
> +      );
> +    }

let docs = [];
    for (let iframe, doc = document; doc; doc = iframe && iframe.contentDocument) {
      docs.push(doc);
      iframe = doc.querySelector("iframe");
    }

    return docs.map(doc => doc.querySelector("pre").textContent);

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow_reload.js:254
(Diff revision 12)
> +        if (details.tabId == activeTabId && details.frameId == 0) {
> +          reloads++;
> +
> +          switch (reloads) {
> +            case 1:
> +              browser.tabs.executeScript({code: `(${checkInjectedScript})(false)`});

await executeScript(...);

::: browser/components/extensions/test/browser/file_inspectedwindow_reload_target.sjs:1
(Diff revision 12)
> +Components.utils.importGlobalProperties(["URLSearchParams"]);

Please add modeline headers.

::: browser/components/extensions/test/browser/file_inspectedwindow_reload_target.sjs:46
(Diff revision 12)
> +function handleInjectedScriptTestRequest(request, response, params) {
> +  response.setHeader("Content-Type", "text/html; charset=UTF-8", false);
> +
> +  const frames = parseInt(params.get("frames"));
> +  let content = "";
> +

Unnecessary blank line.

::: browser/components/extensions/test/browser/file_inspectedwindow_reload_target.sjs:64
(Diff revision 12)
> +       </style>
> +      </head>
> +      <body>
> +       <h1>IFRAME ${frames}</h1>
> +       <pre>injected script NOT executed</pre>
> +       <script>

type="text/javascript"
Attachment #8788244 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76808/#review107186

r=me with comments addressed.

::: browser/components/extensions/ext-devtools-inspectedWindow.js:21
(Diff revision 11)
> +    if (inspectedWindowFront) {
> +      return Promise.resolve(inspectedWindowFront);
> +    }
> +
> +    // If there is not yet a front instance, then a lazily cloned target for the context is
> +    // retrieved using the DevtoolsParentContextsManager helper (which is an asynchronous operation,
> +    // because the first time that the target has been cloned, it is not ready to be used to create
> +    // the front instance until it is connected to the remote debugger successfully).
> +    return getDevToolsTargetForContext(context)
> +      .then(clonedTarget => {
> +        inspectedWindowFront = new WebExtensionInspectedWindowFront(clonedTarget.client,
> +                                                                    clonedTarget.form);
> +        return inspectedWindowFront;
> +      });

There's a race here. Please make `inspectedWindowFront` a promise, and store the promise that receives it rather than the value.

::: browser/components/extensions/ext-devtools-inspectedWindow.js:40
(Diff revision 11)
> +  }
> +
> +  // TODO(rpl): retrive a more detailed callerInfo object, like the filename and
> +  // lineNumber of the actual extension called, in the child process.
> +  const callerInfo = {
> +    addonId: context.extension.uuid,

Shouldn't this be the real add-on ID?

::: browser/components/extensions/ext-devtools-inspectedWindow.js:47
(Diff revision 11)
> +  };
> +
> +  return {
> +    devtools: {
> +      inspectedWindow: {
> +        eval(expression, options) {

Please make this an async function.

::: browser/components/extensions/ext-devtools-inspectedWindow.js:52
(Diff revision 11)
> +        eval(expression, options) {
> +          return getInspectedWindowFront().then(inspectedWindowFront => {
> +            return inspectedWindowFront.eval(callerInfo, expression, options || {}).then(evalResult => {
> +              // TODO(rpl): check for additional undocumented behaviors on chrome
> +              // (e.g. if we should also print error to the console or set lastError?).
> +              return Promise.resolve(new SpreadArgs([evalResult.value, evalResult.exceptionInfo]));

No need for `Promise.resolve`

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:136
(Diff revision 11)
> +        browser.test.sendMessage("inspectedWindow-eval-result", {
> +          evalResult,
> +          errorResult,
> +        });
> +      } catch (err) {
> +        browser.test.sendMessage("inspectedWindow-eval-result", undefined);

No need to pass the `undefined` argument.

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:137
(Diff revision 11)
> +          evalResult,
> +          errorResult,
> +        });
> +      } catch (err) {
> +        browser.test.sendMessage("inspectedWindow-eval-result", undefined);
> +        throw err;

Don't throw this. Just report it with `browser.test.fail`

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:153
(Diff revision 11)
> +      <html>
> +       <head>
> +         <meta charset="utf-8">
> +       </head>
> +       <body>
> +         <script src="devtools_page.js"></script>

`type="text/javascript"` and please move to the <head>.

::: browser/components/extensions/test/browser/browser_ext_devtools_inspectedWindow.js:217
(Diff revision 11)
> +        const expectedErrorPropValue = expectedResults.errorResult[errorPropName];
> +        const actualErrorPropValue = errorResult[errorPropName];

Please use shorter variable names.
Attachment #8788242 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76808/#review107186

> Please make this an async function.

do you mean async function as in ES6 async/await or as in "async": "callback"?

(if it is the latter: this is already an "WebExtension async function", but the "evalError or evalException info" is reported as a rejection of the returned promise, mostly because on Chrome it is the second parameter of the related callback and it is not set as a lastError and I would prefer to keep the same convention for the returned promise).
Comment on attachment 8788242 [details]
Bug 1300584 - Implements devtools.inspectedWindow.eval.

https://reviewboard.mozilla.org/r/76808/#review107186

> There's a race here. Please make `inspectedWindowFront` a promise, and store the promise that receives it rather than the value.

Thanks! great catch! and the updated version is even nicer.
Comment on attachment 8788244 [details]
Bug 1300584 - Implements devtools.inspectedWindow.reload.

https://reviewboard.mozilla.org/r/76812/#review107172

> s/inspectedWindowFront/win/g

I opted for `front` (also for the similar code in the other patch), mostly because we usually use `win` as a shorter name for a `window` object and this is something totally different (and `front` is the terminology often used in the devtools clients to refer to this `ActorFronts` instances).
(In reply to Luca Greco [:rpl] from comment #54)
> Comment on attachment 8788242 [details]
> Bug 1300584 - Implements devtools.inspectedWindow.eval.
> 
> https://reviewboard.mozilla.org/r/76808/#review107186
> 
> > Please make this an async function.
> 
> do you mean async function as in ES6 async/await or as in "async":
> "callback"?
> 
> (if it is the latter: this is already an "WebExtension async function", but
> the "evalError or evalException info" is reported as a rejection of the

There was a typo here, what I meant is:

the "evalError or evalException info" is NOT reported as a rejection
Patches updates:
- rebased on a recent mozilla-central tip
- fixed the last mozreview comment (by change eval and reload into async functions)
- fixed new eslint error (prefer '{once: true}' on addEventListener/removeEventListener)

One more push to try:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=744aa7f8697bf1e97998e268fc6370c796e3fab1
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c20c6b11a1da
Implements devtools.inspectedWindow.eval. r=kmag
https://hg.mozilla.org/integration/autoland/rev/0a699117d214
Implements devtools.inspectedWindow.reload. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c20c6b11a1da
https://hg.mozilla.org/mozilla-central/rev/0a699117d214
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: