Alexandre Poirot [:ochameau] Assignee
	Description • 10 years ago

Following up on bug 609872. Now we can select a specific iframe context for the console, but it doesn't help with any other tool.
We should have a way to target a given iframe for the whole toolbox.
It is especially important when you start opening Gaia in a firefox tab, as Gaia spawns tons of iframes and you want to focus on only one app frame.
Also, on Firefox OS, apps spawn multiple iframes in their processes. For now, we can only target the main app frame. But we would like to be able to target all others. It ends up being very close to implement the "iframe selection" feature being described over bug 609872.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 1 • 10 years ago

Really excited to see this worked on!

One question for Alex: the drop-list is nice for mouse-driven developers, but I think it should be possible to open the list and select frames entirely from the keyboard as well.

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 10 years ago

Works quite well now both on desktop with a webpage having multiple iframes,
also with gaia running in firefox as well as with special app frames running in child processes!

Next step: having some tests...

	Alexandre Poirot [:ochameau] Assignee
	Comment 4 • 10 years ago

Joe, Paul told me you could help me drive this forward...
The patch works really great now and is ready for review!
But I need help about:
1) Approving the UI/UX we are about to add to the toolbox.
This patch looks exactly like what I demoed during the workweek (without the Mulet bad-looking chrome skin). It displays a button in between responsive mode and sidebar buttons.
This button is only displayed if there is an iframe in the current document, otherwise it is kept hidden. The button label is just "Frames".
When you click on it, a menupopup appears with the list of all iframes URLs.
Then, when you click on one menuitem, the whole toolbox is going to target the related iframe context.
The iframe list itself always stays the same, so that you can always get back to the original web page context. And everytime a frame is created/destroyed, the list is updated.

2) Finding reviewers!
I think we can already start reviewing and landing most of the patch and figure out the UX in parallel by only postponing the landing of the toolbox.js modification.
Can you take a brief look at the patch and tell me who is/are good candidate(s) to review it?
I can split this patch in multiple meaningfull/independant pieces, if that can help:
 * DebuggerProgressListener factorization between "old actors" (webbrowser,webconsole) and protocol.js ones (inspector)
 * Server-side tweaks in webbrowser.js to send messages to the client about new/updated/destroyed frames
 * Client-side tweaks to listen to these messages and display a UI in the toolbox
 * random fixes in script.js and markup-view.js
 * tests

	Joe Walker [:jwalker]
	Comment 5 • 10 years ago

I've needinfo?ed Darrin who can help you with a UI review. To help him out, please could you add a screenshot?

Could you ask Brian for a review, and also make sure that Panos and Mihai are aware of what's going on - maybe give them an f?

My slight worry is that there parts of devtools that make subtle assumptions that are broken by this patch. Can it wait until 31?

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 6 • 10 years ago

(In reply to Joe Walker [:jwalker] from comment #5)
...
> My slight worry is that there parts of devtools that make subtle assumptions
> that are broken by this patch. Can it wait until 31?

I have the same worry in particular because bugs may not be obviously associated with this feature. 

The way to mitigate the worry is to thoroughly test the feature. Pushing landing to after uplift might be best. When we land I'd suggest that you post to the devrel and devtools lists ideally with a specific set of test scenarios, and ask people to kick the tires and log bugs.

	Alexandre Poirot [:ochameau] Assignee
	Comment 7 • 10 years ago

(In reply to Joe Walker [:jwalker] from comment #5)
> I've needinfo?ed Darrin who can help you with a UI review. To help him out,
> please could you add a screenshot?

I'll do whenever I find time for this.

> 
> Could you ask Brian for a review, and also make sure that Panos and Mihai
> are aware of what's going on - maybe give them an f?

Ok!

> 
> My slight worry is that there parts of devtools that make subtle assumptions
> that are broken by this patch. Can it wait until 31?

Code-wise, this feature isn't that stressful. I'm mainly hooking into tabNavigated event so that switching to a different iframe isn't much different from switching from website A to website B in the same tab. But yes, this features needs proper testing for each tool. That's something I need help as I don't know much about each of them and I can't immediatly spot all possible tool-specific regressions...

I just wish we could land the necessary device patch to gecko-30 in order to have this feature in b2g 1.4. Again I'm pushing on this feature more for Firefox OS/Apps developers than regular web developers. I'm fine if there is some delay in landing the final UI part in Firefox or having it hidden for a bit behind a pref.
Also I do have too much on my plate with this workweek effort. I'd prefer spending some extra efforts right now and ship that instead of seeing this patch rot.

I'll try to split the patch this week and start asking for reviews for non-user facing patches.

	Joe Walker [:jwalker]
	Comment 8 • 10 years ago

You're right, it's not as invasive as I thought it was going to be. No need to wait until 31.

	Alexandre Poirot [:ochameau] Assignee
	Comment 10 • 10 years ago

Mihai, I've seen various exceptions when trying to set a breakpoint in
gaia codebase. That's because when you use window.open() in gaia,
it will open a really special window, in another process.
It ends up modifying the final behavior of nsIDOMWindow/nsIDOMDocument.
In this particular case, window.location and document.location were null.
Without this patch, the breakpoint fails.

	Alexandre Poirot [:ochameau] Assignee
	Comment 11 • 10 years ago

Joe, Two things here:
* First the additional call to done(). We really have to call the function returned
by this._inspector.updating(...), otherwise the selection will be just broken.
(updating() breaks if any of its caller forget to call its returned function)
* For some reason, 'this._containers.get(parent)' can end up being undefined.
I don't know much about all this code, but that prevents breaking the selection.
(the exception was preventing calling updating() callback...)

This exception happens in the mochitest-chrome test included in the UI patch.

	Alexandre Poirot [:ochameau] Assignee
	Comment 12 • 10 years ago

Panos, A very simple tweak to improve debugging actors.
When an exception happens during actor loading, we weren't getting the precise line where it throws...

	Joe Walker [:jwalker]
	Comment 13 • 10 years ago

Comment on attachment 8389210 [details] [diff] [review]
Prevent markupview from breaking selection.

Review of attachment 8389210 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/markupview/markup-view.js
@@ +290,5 @@
>          if (selection.reason !== "treepanel") {
>            this.markNodeAsSelected(selection.nodeFront);
>          }
>          done();
> +      }, done);

Minor tweak:

this.showNode(selection.nodeFront, true).then(() => {
  if (selection.reason !== "treepanel") {
    this.markNodeAsSelected(selection.nodeFront);
  }
}).then(done, console.error);

?

	Mihai Sucan [:msucan]
	Comment 14 • 10 years ago

Comment on attachment 8389204 [details] [diff] [review]
Prevent exceptions in debugger when inspecting b2g popup window object.

Review of attachment 8389204 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

	Alexandre Poirot [:ochameau] Assignee
	Comment 15 • 10 years ago

Brian, This patch is a necessary refactoring for frames support.
Currently we have some duplicated code between webbrowser.js and inspector.js.
In both of these files, we are listening for webProgress events in order
to support new document loading.
But the thing is that frame support will rely on the same code.
Switching to another frame is more or less the same thing than opening a link
and switching to a new document.

So, in order to ease faking the navigate/will-navigate (TabActor, WebConsole) and the windowchange-start/windowchange-stop (Inspector),
I've factorized this code into only one in webbrowser.
Some facets of this patch may look useless, like watch/unwatch, or the onLocationChangeStart/onLocationChangeStop,
but they will be useful in the following patch, that will introduce frames support.

Mihai, I ended up modifying what you just introduced for the cd() command.
Can you take a quick look at this patch?

	Alexandre Poirot [:ochameau] Assignee
	Comment 16 • 10 years ago

Server side - toolkit patch.

This patch hooks into tabNavigated message for updating the client,
and on the previous patch and windowchange-start/windowchange-stop events for the server side.
It also introduces some new messages (tabDocShellUpdate) being sent to the child
in order to notify about the existing frames.
TabActor supports a new request (switchToDocShell) in order to change the currently targeted context for the toolbox.
(that will end up dispatching tabNavigated events)

	Alexandre Poirot [:ochameau] Assignee
	Comment 17 • 10 years ago

UI/browser patch.

That patch adds a button to the toolbox, with the list of all existing iframes in the document.
Then you can select one and change the whole toolbox context.

On hold until other patches get r+ and wait for some UX feedback.

	Alexandre Poirot [:ochameau] Assignee
	Comment 18 • 10 years ago

I haven't really tried to optimize the design of this button,
I focused on delivering the feature.
The behavior of this button so far is that it is only shown when there is some iframes in the current document.
Then when you click on it, you have a menu with a list of URL, each iframe url. You also have the website url, in order to be able to get back to it when you switched to an iframe.

I'm also attaching a screenshot of the menu.

	Mihai Sucan [:msucan]
	Comment 20 • 10 years ago

Comment on attachment 8389320 [details] [diff] [review]
Use same ProgressListener event across actors

Console-related changes look good to me

	Alexandre Poirot [:ochameau] Assignee
	Comment 21 • 10 years ago

I had to fix these two things to get greener tbpl:
 - forgot to remove observer service listener (related to attachment 8389327 [details] [diff] [review])
 - ensure that inspector tests attach to TabActor (in order to receive windowchange-start/stop)
(related to attachment 8389320 [details] [diff] [review])

https://tbpl.mozilla.org/?tree=Try&rev=49b13c4fb3d0

	Alexandre Poirot [:ochameau] Assignee
	Comment 22 • 10 years ago

Updated with last interdiff.

	Alexandre Poirot [:ochameau] Assignee
	Comment 23 • 10 years ago

Updated with last interdiff

	Alexandre Poirot [:ochameau] Assignee
	Comment 24 • 10 years ago

Comment on attachment 8389884 [details] [diff] [review]
Use same ProgressListener event across actors

Brian, feel free to request anyone else to take a look, if you don't feel confident reviewing all this code.

	Brian Grinstead [:bgrins]
	Comment 25 • 10 years ago

(In reply to Alexandre Poirot (:ochameau) from comment #24)
> Comment on attachment 8389884 [details] [diff] [review]
> Use same ProgressListener event across actors
> 
> Brian, feel free to request anyone else to take a look, if you don't feel
> confident reviewing all this code.

FYI - Both of these patches need to be rebased since they have rejects on the file toolkit/devtools/server/actors/webbrowser.js

	Brian Grinstead [:bgrins]
	Comment 26 • 10 years ago

Comment on attachment 8389884 [details] [diff] [review]
Use same ProgressListener event across actors

Review of attachment 8389884 [details] [diff] [review]:
-----------------------------------------------------------------

Panos, can you please take a look at the changes in webbrowser.js

	Brian Grinstead [:bgrins]
	Comment 27 • 10 years ago

Comment on attachment 8389884 [details] [diff] [review]
Use same ProgressListener event across actors

Review of attachment 8389884 [details] [diff] [review]:
-----------------------------------------------------------------

I think the change to merge the inspector progress listener with the one used in other tools is a good idea, and am happy with most of the changes in inspector.js.  I've left a comment on one part that I'm not understanding in the changes.

::: toolkit/devtools/server/actors/inspector.js
@@ +1890,5 @@
>      }
>    },
>  
> +  onFrameLoad: function(window, isTopLevel) {
> +    if (!this.rootDoc && isTopLevel) {

Triggering a newRoot mutation on a frameLoad seems that it would cause issues with the various inspector tools.  Anywhere that calls walker.getRootNode() would then use the inner window automatically.

The places that this happens are:
1) Font inspector / showAll
https://mxr.mozilla.org/mozilla-central/source/browser/devtools/fontinspector/font-inspector.js#205
2) Inspector panel / _getDefaultNodeForSelection: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#209

In either of these cases, if the rootNode/rootDoc belonged to the inner frame, they would operate as if this frame was the main window.  For example, the font inspector would show all the fonts used inside of the last frame that happened load.

Is there a reason for removing this condition?

	Alexandre Poirot [:ochameau] Assignee
	Comment 28 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #27)
> Comment on attachment 8389884 [details] [diff] [review]
> ::: toolkit/devtools/server/actors/inspector.js
> @@ +1890,5 @@
> >      }
> >    },
> >  
> > +  onFrameLoad: function(window, isTopLevel) {
> > +    if (!this.rootDoc && isTopLevel) {
> 
> Triggering a newRoot mutation on a frameLoad seems that it would cause
> issues with the various inspector tools.  Anywhere that calls
> walker.getRootNode() would then use the inner window automatically.

I only removed the `!frame` as that looks redundant with `isTopLevel`.
So that it should not change when we enter in this condition at the end.
But it will allow accepting switching to an iframe as new root
if the user explicitely asked to switch to an iframe.
In such scenario, we fake a windowchange-stop event on the selected iframe,
and report the iframe as being top-level.
Otherwise, in regular scenario, if we browse a website with an iframe, it will fire onFrameLoad for the iframe, but isTopLevel will be false.

> In either of these cases, if the rootNode/rootDoc belonged to the inner
> frame, they would operate as if this frame was the main window.  For
> example, the font inspector would show all the fonts used inside of the last
> frame that happened load.

Does the previous comment clarify this or I misunderstood your concern?
What you describe here is exactly what I want to achieve for toolbox-level iframe selection.

	Alexandre Poirot [:ochameau] Assignee
	Comment 29 • 10 years ago

Rebased patch.

	Alexandre Poirot [:ochameau] Assignee
	Comment 30 • 10 years ago

Rebased patch.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 31 • 10 years ago

Comment on attachment 8392412 [details] [diff] [review]
Use same ProgressListener event across actors

Review of attachment 8392412 [details] [diff] [review]:
-----------------------------------------------------------------

Kudos for trying to refactor one of the hairiest parts of the debugger, which has been providing us with a steady supply of bugs all this time! I have a few questions that need clarification before I can give this an r+.

While playing with all the patches applied, I got the following error when selecting another iframe in http://htmlpad.org/debugger-iframes/:

Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "[JavaScript Error: "this._tabbrowser._getTabForContentWindow(...) is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js" line: 1189}]'[JavaScript Error: "this._tabbrowser._getTabForContentWindow(...) is null" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js" line: 1189}]' when calling method: [nsIRunnable::run]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: yes]

Can you add a null check for this in BrowserTabActor.prototype.title while you are here?

::: toolkit/devtools/server/actors/webbrowser.js
@@ +932,5 @@
> +   * Start notifying server codebase and client about a new document
> +   * being loaded in the currently targeted context.
> +   */
> +  _onLocationChangeStart: function (newURI) {
> +    this.threadActor.clearDebuggees();

Where did that come from? We already clear debuggees in onWindowCreated (among other things). If this is necessary for debugging iframes, why didn't you reuse the rest?

@@ +935,5 @@
> +  _onLocationChangeStart: function (newURI) {
> +    this.threadActor.clearDebuggees();
> +    this.threadActor.disableAllBreakpoints();
> +
> +    events.emit(this, "windowchange-start", this.window, true);

To be honest I think will-navigate/navigate are more clear and concise as event names than windowchange-start/windowchange-stop windowchange is too broad, in the abstract it could even mean changes in the window DOM. I'm not going to push for this though if you disagree.

@@ +954,5 @@
> +    let threadActor = this.threadActor;
> +    threadActor.global = this.window.wrappedJSObject ? this.window.wrappedJSObject : this.window;
> +    if (threadActor.attached) {
> +      threadActor.findGlobals();
> +    }

Same question as before, why is this needed? This is another bit of onWindowCreated, but again not all of it. Assuming that onWindowCreated is only registered for the top-level window, can't you just arrange for it to be used for child frames as well?

Alternatively, if the above is not feasible, can't you consolidate the entire functionality of onWindowCreated into the webProgress listener?

@@ +1275,5 @@
>  
> +  watch: function DPL_watch(aProgress) {
> +    aProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_LOCATION |
> +                                        Ci.nsIWebProgress.NOTIFY_STATE_WINDOW |
> +                                        Ci.nsIWebProgress.NOTIFY_STATE_NETWORK);

Previously we would listen for NOTIFY_STATE_ALL in the tab actor and NOTIFY_ALL in the inspector actor, which I'm glad to see optimized in this patch. But we were explicitly testing against NOTIFY_STATE_DOCUMENT in inspector.js and this change will ignore those state transitions, unless I'm missing something.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 32 • 10 years ago

Comment on attachment 8392414 [details] [diff] [review]
Tweak TabActor to support changing its targeted context to an iframe

Review of attachment 8392414 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have many comments on this one, but testing the patches on http://htmlpad.org/debugger-iframes/ causes errors on style editor and highlighter when switching iframes. Let me know if you can't reproduce them, but it should be fairly straightforward.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +693,5 @@
> +    // We watch for all child docshells under the current document,
> +    this._progressListener.watch(this.webProgress);
> +
> +    // And list all already existing ones.
> +    var containedDocShells = this.webProgress.getDocShellEnumerator(

s/var/let/

@@ +697,5 @@
> +    var containedDocShells = this.webProgress.getDocShellEnumerator(
> +                               Ci.nsIDocShellTreeItem.typeAll,
> +                               Ci.nsIDocShell.ENUMERATE_FORWARDS);
> +    while (containedDocShells.hasMoreElements()) {
> +      var childDocShell = containedDocShells.getNext();

s/var/let/

@@ +712,5 @@
> +    try {
> +      win = Services.wm.getOuterWindowWithId(windowId);
> +    } catch(e) {}
> +    if (!win) {
> +      return { error: "The related docshell is destroyed" };

"... or not found" (if windowId is invalid).

@@ +745,5 @@
> +      let webProgress = aSubject.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                .getInterface(Ci.nsIWebProgress);
> +      let id = webProgress.DOMWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                          .getInterface(Ci.nsIDOMWindowUtils)
> +                          .outerWindowID;

id isn't used anywhere.

@@ +759,5 @@
> +                     type: "tabDocShellUpdate",
> +                     windowId: id,
> +                     url: window.location.href,
> +                     title: window.title
> +                   });

Can we include the windowId of the parent so that the client has enough information to generate the docshell tree? Right now it can only generate a list as can bee seen at http://htmlpad.org/debugger-iframes/

@@ +1097,5 @@
> +                            .getInterface(Ci.nsIWebNavigation)
> +                            .QueryInterface(Ci.nsIDocShell);
> +    // Here is the very important call where we switch the currently
> +    // targeted context.
> +    Object.defineProperty(this, "docShell", { value: docShell, enumerable: true, configurable: true });

Nit: take care of lines exceeding 80 chars in this method.

@@ +1497,5 @@
> +                                           this._tabActor._originalWindow :
> +                                           this._tabActor.window);
> +
> +      // If we change the top level document, we just reseted the docshell list
> +      // so we need to refresh it now

Nit: "If we are changing the top level document, we just reset the docshell list, so we need to refresh it now."

	Brian Grinstead [:bgrins]
	Comment 33 • 10 years ago

Comment on attachment 8392412 [details] [diff] [review]
Use same ProgressListener event across actors

Review of attachment 8392412 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Alexandre Poirot (:ochameau) from comment #28) 
> I only removed the `!frame` as that looks redundant with `isTopLevel`.
> So that it should not change when we enter in this condition at the end.
> But it will allow accepting switching to an iframe as new root
> if the user explicitely asked to switch to an iframe.
> In such scenario, we fake a windowchange-stop event on the selected iframe,
> and report the iframe as being top-level.
> Otherwise, in regular scenario, if we browse a website with an iframe, it
> will fire onFrameLoad for the iframe, but isTopLevel will be false.

OK, makes sense.  If LH.isTopLevelWindow is true then LH.getFrameElement will always be null, but with the changes in the patch isTopLevel (the function argument) could be true with LH.getFrameElement returning an object.  r+ for the inspector changes

	Alexandre Poirot [:ochameau] Assignee
	Comment 34 • 10 years ago

When you open a page that triggers bug 285395
like this page: http://htmlpad.org/debugger-iframes/
You end up with a broken style editor.

That's because it creates iframes without contentDocument attribute.

	Alexandre Poirot [:ochameau] Assignee
	Comment 35 • 10 years ago

Based on top of attachment 8392412 [details] [diff] [review].

	Alexandre Poirot [:ochameau] Assignee
	Comment 36 • 10 years ago

In this new patch, I should have addressed all review comments.
Your comment about the fact that I took part of DOMWindowCreated actions regarding
threadActor and mixed them into will-navigate/navigate actions was relevant!
In this new patch, I also factor out the DOMWindowCreated listener,
so that, when I switch to an iframe, I fake a call to will-navigate, DOMWindowcreated and navigate.
At the end, the patch shouldn't change what we were doing for all these three events,
it will only allow us to easily fake such call when switching the frame!

Here is a try run for just this changeset and all the already r+ small patches:
https://tbpl.mozilla.org/?tree=Try&rev=12ac58a378bd

	Alexandre Poirot [:ochameau] Assignee
	Comment 37 • 10 years ago

Comment on attachment 8394261 [details] [diff] [review]
Prevent styleeditor to break when iframes don't have a document

See comment 34.

	Alexandre Poirot [:ochameau] Assignee
	Comment 38 • 10 years ago

Based on attachment 8392414 [details] [diff] [review].

	Alexandre Poirot [:ochameau] Assignee
	Comment 39 • 10 years ago

Mostly a rebase to gain from the new DOMWindowCreated abstraction.
Still have to address review comments.

	Alexandre Poirot [:ochameau] Assignee
	Comment 40 • 10 years ago

Comment on attachment 8395761 [details] [diff] [review]
Use same ProgressListener event across actors

Review of attachment 8395761 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/webbrowser.js
@@ +903,5 @@
>     * debugging, which may have been disabled temporarily by the
>     * DebuggerProgressListener.
>     */
> +  _onWindowReady: function (window, isTopLevel) {
> +    let isTopLevel = window == this.window;

I failed during a rebase over here, I fixed just that I pushed a new try:
https://tbpl.mozilla.org/?tree=Try&rev=736bb5059c76

	Tim Nguyen :ntim
	Comment 41 • 10 years ago

(In reply to Alexandre Poirot (:ochameau) from comment #18)
> Created attachment 8389333 [details]
> The frames menu button
> 
> I haven't really tried to optimize the design of this button,
> I focused on delivering the feature.
> The behavior of this button so far is that it is only shown when there is
> some iframes in the current document.
> Then when you click on it, you have a menu with a list of URL, each iframe
> url. You also have the website url, in order to be able to get back to it
> when you switched to an iframe.
> 
> I'm also attaching a screenshot of the menu.

I recommend you to use the command-button class. It makes all the styling ready for you.

	Brian Grinstead [:bgrins]
	Comment 42 • 10 years ago

Comment on attachment 8395761 [details] [diff] [review]
Use same ProgressListener event across actors

Review of attachment 8395761 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the inspector bits once the extra arguments are removed from the code

::: toolkit/devtools/server/actors/inspector.js
@@ +1943,5 @@
>      }
>      return false;
>    },
>  
> +  onFrameUnload: function(window, isTopLevel, newURI, request) {

There are new arguments added to this function that are not being used

	Alexandre Poirot [:ochameau] Assignee
	Comment 43 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/e7e7dbd8760c
https://hg.mozilla.org/integration/fx-team/rev/277a50ae937a
https://hg.mozilla.org/integration/fx-team/rev/409d9fa43f9b
https://hg.mozilla.org/integration/fx-team/rev/f75aaf376eeb

	Alexandre Poirot [:ochameau] Assignee
	Comment 44 • 10 years ago

Addressed review comment.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 45 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/e7e7dbd8760c
https://hg.mozilla.org/mozilla-central/rev/277a50ae937a
https://hg.mozilla.org/mozilla-central/rev/409d9fa43f9b
https://hg.mozilla.org/mozilla-central/rev/f75aaf376eeb

	Girish Sharma [:Optimizer]
	Comment 46 • 10 years ago

IIANW, this is to be leave-open

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 47 • 10 years ago

I don't think I can finish the review today, sorry. I'll make sure to finish it on Monday.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 48 • 10 years ago

Comment on attachment 8395761 [details] [diff] [review]
Use same ProgressListener event across actors

Review of attachment 8395761 [details] [diff] [review]:
-----------------------------------------------------------------

In general this version worked almost flawlessly, but there is one regression and enough comments that make me want to take another look. Sorry it took a while, but this is the kind of code that I need to review patiently and with a clear head.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +526,5 @@
>      throw "The docShell getter should be implemented by a subclass of TabActor";
>    },
>  
> +  get chromeEventHandler() {
> +    // TODO: fix docShell.chromeEventHandler in child processes!

This is done in the next patch, right? We can't break that stuff.

@@ +903,5 @@
>     * debugging, which may have been disabled temporarily by the
>     * DebuggerProgressListener.
>     */
> +  _onWindowReady: function (window, isTopLevel) {
> +    let isTopLevel = window == this.window;

Where does the isTopLevel argument come from? Probably a typo?

I'm also not too keen on the onFoo pattern for _onWindowReady, _onWillNavigate and _onNavigate. onFoo implies an event listener and these are not. How about we called them _doWindowReady/_doNavigate/_doWillNavigate?

@@ +905,5 @@
>     */
> +  _onWindowReady: function (window, isTopLevel) {
> +    let isTopLevel = window == this.window;
> +    events.emit(this, "window-ready", window, isTopLevel);
> +    dumpn("window-ready: " + window.location + " isTopLevel:" + isTopLevel);

Don't forget to remove this. You might also want to dump() before emitting the event, so that there is a log that event listeners have been called.

@@ +907,5 @@
> +    let isTopLevel = window == this.window;
> +    events.emit(this, "window-ready", window, isTopLevel);
> +    dumpn("window-ready: " + window.location + " isTopLevel:" + isTopLevel);
> +
> +    // TODO: move that code to ThreadActor by listening to window-ready

If this is not done in the next patch, then please add the followup bug number in the comment.

@@ +913,5 @@
> +    if (isTopLevel) {
> +      threadActor.clearDebuggees();
> +      if (threadActor.dbg) {
> +        threadActor.dbg.enabled = true;
> +        threadActor.global = window.wrappedJSObject;

This needs rebasing, we no longer waive the XRay wrapper.

@@ +932,5 @@
> +   */
> +  _onWillNavigate: function (window, newURI, request) {
> +    let isTopLevel = window == this.window;
> +
> +    events.emit(this, "will-navigate", window, isTopLevel, newURI, request);

This is becoming unwieldy. How about we stick to the common pattern of a single object payload and use destructuring in the event listener?

@@ +933,5 @@
> +  _onWillNavigate: function (window, newURI, request) {
> +    let isTopLevel = window == this.window;
> +
> +    events.emit(this, "will-navigate", window, isTopLevel, newURI, request);
> +    dumpn("will-navigate: " + newURI + " isTopLevel:" + isTopLevel);

Same comment about dump() as above.

@@ +942,5 @@
> +      return;
> +    }
> +
> +    // Proceed normally only if the debuggee is not paused.
> +    // TODO: move that code to ThreadActor by listening to will-navigate

If we just do what the comment says, then this code will get called even when isTopLevel == false, since we emit the event before the check.

There is a subtle and more fundamental problem with that though. Using an event to call this code will implicitly rely on the event emitter implementation being sync, which is not a good assumption to make, as the ongoing promise refactoring demonstrated. If we ever switch to an async event emitter (and we've talked about that before) then the thread actor code could run too late. Especially since an event listener is not guaranteed a certain spot in the listener queue, making it possible to run after all the other listeners have taken their turn.

@@ +965,5 @@
> +  /**
> +   * Notify server and client about a new document done loading in the current
> +   * targeted context.
> +   */
> +  _onNavigate: function (window) {

The same comments as for _onWillNavigate apply here, too.

@@ +991,5 @@
> +      title: this.title,
> +      nativeConsoleAPI: this.hasNativeConsoleAPI(this.window),
> +      state: "stop"
> +    });
> +  },

Another change from the previous behavior is that we now emit the navigate/will-navigate events before doing the actual work, instead of after. Why is that? I'm not sure if it breaks anything currently (there are hardly any subscribers to these events), but it doesn't make sense to me anyway: "load" events are dispatched after the page loads, not before.

@@ +1300,5 @@
>  
> +  watch: function DPL_watch(docShell) {
> +    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                              .getInterface(Ci.nsIWebProgress);
> +    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATUS |

Where did NOTIFY_STATUS come from? We used to filter NOTIFY_STATE_ALL here (but only use NOTIFY_STATE_WINDOW and NOTIFY_STATE_NETWORK) and NOTIFY_STATE_WINDOW/NOTIFY_STATE_DOCUMENT in the inspector actor. I don't see any related onStatusChange handler in the patch.

@@ +1333,5 @@
> +  },
> +
> +  onWindowCreated:
> +  DevToolsUtils.makeInfallible(function DPL_onWindowCreated(evt) {
> +    // Ignore any event if the tab actor isn't attached

Nit: missing full-stop.

@@ +1352,3 @@
>    onStateChange:
>    DevToolsUtils.makeInfallible(function DPL_onStateChange(aProgress, aRequest, aFlag, aStatus) {
> +    // Ignore any event if the tab actor isn't attached

Nit: missing full-stop.

@@ +1368,3 @@
>      }
> +    if (isWindow && isStop) {
> +      this._tabActor._onNavigate(window);

This is what I was initially mostly worried about, since we are now using the inspector actor's state changes instead of the tab actor's. It seems that functionality is mostly unaffected, with one exception, which I'm not entirely sure it's because of this hunk.

STR:
1. Go to  http://htmlpad.org/debugger and open the debugger.
2. Set a breakpoint at line 125.
3. Reload and examine the packet trace in the terminal. Somehow tabNavigated:stop is logged before tabNavigated:start.

	Alexandre Poirot [:ochameau] Assignee
	Comment 49 • 10 years ago

(In reply to Panos Astithas [:past] from comment #48)
> Comment on attachment 8395761 [details] [diff] [review]
> ::: toolkit/devtools/server/actors/webbrowser.js
> @@ +526,5 @@
> >      throw "The docShell getter should be implemented by a subclass of TabActor";
> >    },
> >  
> > +  get chromeEventHandler() {
> > +    // TODO: fix docShell.chromeEventHandler in child processes!
> 
> This is done in the next patch, right? We can't break that stuff.

It won't be broken, I opened the followup, bug 977043. That's just to fix docShell.chromeEventHandler. Today, it returns null in child processes. Here I workaround with that:
  this.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
               .getInterface(Ci.nsIContentFrameMessageManager)
It magically returns the chrome event handler in child processes.
Once bug 977043 lands, we can do: get chromeEventHandler() this.docShell.chromeEventHandler

> 
> @@ +903,5 @@
> >     * debugging, which may have been disabled temporarily by the
> >     * DebuggerProgressListener.
> >     */
> > +  _onWindowReady: function (window, isTopLevel) {
> > +    let isTopLevel = window == this.window;
> 
> Where does the isTopLevel argument come from? Probably a typo?

Yes I mixed with the following patch.
I'll try to clean that up.


> 
> I'm also not too keen on the onFoo pattern for _onWindowReady,
> _onWillNavigate and _onNavigate. onFoo implies an event listener and these
> are not. How about we called them _doWindowReady/_doNavigate/_doWillNavigate?

This time, having "do" sounds weird to me as we don't do navigate.
We just process the fact that a document navigates.
What about just _windowReady/_navigate/_willNavigate?


> @@ +932,5 @@
> > +   */
> > +  _onWillNavigate: function (window, newURI, request) {
> > +    let isTopLevel = window == this.window;
> > +
> > +    events.emit(this, "will-navigate", window, isTopLevel, newURI, request);
> 
> This is becoming unwieldy. How about we stick to the common pattern of a
> single object payload and use destructuring in the event listener?

Ok, I wasn't aware of this common practice. Looks like I keep receive this request on all my patches :p


> @@ +942,5 @@
> > +      return;
> > +    }
> > +
> > +    // Proceed normally only if the debuggee is not paused.
> > +    // TODO: move that code to ThreadActor by listening to will-navigate
> 
> If we just do what the comment says, then this code will get called even
> when isTopLevel == false, since we emit the event before the check.

(listener can still filter by checking `window != this.parentActor.window` 
 [also next patch is going to pass isTopLevel directly to the listeners])

> 
> There is a subtle and more fundamental problem with that though. Using an
> event to call this code will implicitly rely on the event emitter
> implementation being sync, which is not a good assumption to make, as the
> ongoing promise refactoring demonstrated. If we ever switch to an async
> event emitter (and we've talked about that before) then the thread actor
> code could run too late. Especially since an event listener is not
> guaranteed a certain spot in the listener queue, making it possible to run
> after all the other listeners have taken their turn.

That makes me really sad if we can't use a stable event mechanism...
... and instead have to hard code debugger stuff into TabActor.
TabActor should be generic and shouldn't contain any child actors specifics.
For now, webbrowser.js and script.js are completely entangled.
But at least, no matter if we use a event listener, or hardcode a call 
to a thread actor method, we should move this code to ThreadActor
so that everything related to thread actor actually lives in script.js.

Would that work for you if I do that?
Call a new method in threadActor, like _widowReady/_doWindowReady just before the call
to events.emit(...)
Or should I just use a custom internal event machinery that proves it to be synchronous and in expected order? We can for ex iterate over tab actors and


> @@ +991,5 @@
> > +      title: this.title,
> > +      nativeConsoleAPI: this.hasNativeConsoleAPI(this.window),
> > +      state: "stop"
> > +    });
> > +  },
> 
> Another change from the previous behavior is that we now emit the
> navigate/will-navigate events before doing the actual work, instead of
> after. Why is that? I'm not sure if it breaks anything currently (there are
> hardly any subscribers to these events), but it doesn't make sense to me
> anyway: "load" events are dispatched after the page loads, not before.

I'm not convinved it makes any difference as the order of execution of server vs client is going to be hardly shuffled by the network. In most cases the server code will do stuff on willNavigate/navigate before the client receive the related debugger protocol packets. I think it is sane to ensure replicating that order so that we do not get differences between local and remote connections.
Also I do think it is usefull to handle the event on the server *before* the client actually receive the related event. Like switching to a new root node in the inspector... Otherwise the client may race the server while it is still processing the event he just sent.


> 
> @@ +1300,5 @@
> >  
> > +  watch: function DPL_watch(docShell) {
> > +    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                              .getInterface(Ci.nsIWebProgress);
> > +    webProgress.addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATUS |
> 
> Where did NOTIFY_STATUS come from? We used to filter NOTIFY_STATE_ALL here
> (but only use NOTIFY_STATE_WINDOW and NOTIFY_STATE_NETWORK) and
> NOTIFY_STATE_WINDOW/NOTIFY_STATE_DOCUMENT in the inspector actor. I don't
> see any related onStatusChange handler in the patch.

Yes I misread/paste this filter. I was thinking about a NOTIFY_STATE, but that doesn't exists!
So I'll end up with just NOTIFY_STATE_WINDOW/NOTIFY_STATE_DOCUMENT.

> @@ +1368,3 @@
> >      }
> > +    if (isWindow && isStop) {
> > +      this._tabActor._onNavigate(window);
> 
> This is what I was initially mostly worried about, since we are now using
> the inspector actor's state changes instead of the tab actor's. It seems
> that functionality is mostly unaffected, with one exception, which I'm not
> entirely sure it's because of this hunk.
> 
> STR:
> 1. Go to  http://htmlpad.org/debugger and open the debugger.
> 2. Set a breakpoint at line 125.
> 3. Reload and examine the packet trace in the terminal. Somehow
> tabNavigated:stop is logged before tabNavigated:start.

Hum, I'm not seing this behavior. It stops on the breakpoint just after the tabNavigated:start. Then when I resume, I get the tabNavigated:stop. I tested on Firefox against a tab.
Does it fire two tabNavigated event before we hit the breakpoint?

	Alexandre Poirot [:ochameau] Assignee
	Comment 50 • 10 years ago

(In reply to Alexandre Poirot (:ochameau) from comment #49)
> > @@ +1368,3 @@
> > >      }
> > > +    if (isWindow && isStop) {
> > > +      this._tabActor._onNavigate(window);
> > 
> > This is what I was initially mostly worried about, since we are now using
> > the inspector actor's state changes instead of the tab actor's.

So I verified what was the actual differences between TabActor and inspector events.
The first one, will-navigate won't change with this patch. In all my tests, isWindow, isDocument and isStart are all true only in one unique event.
For the second, navigate, it will be different. We first get isStop+isDocument, then we get isStop+isWindow. From my investigation, I would say isStop+isDocument relates to DOMContentLoaded, whereas isStop+isWindow is the load event. I'm basing this assumption by looking at document readyState which is interactive in the first whereas it is complete in the second.
So this change here will fire the tabNavigated event significantly sooner for pages with load of resources as DOMContentLoaded fires way sooner in such scenario.
Having said that, I don't see why it would break anything in the debugger as it doesn't shuffle call order. onWillNavigate is still called *after* DOMWindowCreate/onWindowReady code (all code related to threadActor should be evaluated in the same order) and the debugger shouldn't care about having resources loaded or not?
Also I have the feeling we don't really know why we are waiting for all these particular events. It looks like we just ended up with something that works without really knowing what we were listening for.

	Mihai Sucan [:msucan]
	Comment 51 • 10 years ago

(In reply to Alexandre Poirot (:ochameau) from comment #50)

> Also I have the feeling we don't really know why we are waiting for all
> these particular events. It looks like we just ended up with something that
> works without really knowing what we were listening for.

In the case of the web console we specifically want the following behavior:

- will-navigate fires once navigation starts (all pending user prompts are dealt with), but before the first request starts. This is used to clear console output.

- navigate fires once the document is received from the network. Timing here is not very important.

In both cases these events must only fire for the top level window, not for any resources/sub-documents loaded by the console actor target (the tab).

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 52 • 10 years ago

(In reply to Alexandre Poirot (:ochameau) from comment #49)
> (In reply to Panos Astithas [:past] from comment #48)
> It won't be broken, I opened the followup, bug 977043. That's just to fix
> docShell.chromeEventHandler. Today, it returns null in child processes. Here
> I workaround with that:
>   this.docShell.QueryInterface(Ci.nsIInterfaceRequestor)
>                .getInterface(Ci.nsIContentFrameMessageManager)
> It magically returns the chrome event handler in child processes.
> Once bug 977043 lands, we can do: get chromeEventHandler()
> this.docShell.chromeEventHandler

Perfect.

> What about just _windowReady/_navigate/_willNavigate?

Even better!

> > There is a subtle and more fundamental problem with that though. Using an
> > event to call this code will implicitly rely on the event emitter
> > implementation being sync, which is not a good assumption to make, as the
> > ongoing promise refactoring demonstrated. If we ever switch to an async
> > event emitter (and we've talked about that before) then the thread actor
> > code could run too late. Especially since an event listener is not
> > guaranteed a certain spot in the listener queue, making it possible to run
> > after all the other listeners have taken their turn.
> 
> That makes me really sad if we can't use a stable event mechanism...
> ... and instead have to hard code debugger stuff into TabActor.
> TabActor should be generic and shouldn't contain any child actors specifics.
> For now, webbrowser.js and script.js are completely entangled.
> But at least, no matter if we use a event listener, or hardcode a call 
> to a thread actor method, we should move this code to ThreadActor
> so that everything related to thread actor actually lives in script.js.
> 
> Would that work for you if I do that?
> Call a new method in threadActor, like _widowReady/_doWindowReady just
> before the call
> to events.emit(...)
> Or should I just use a custom internal event machinery that proves it to be
> synchronous and in expected order? We can for ex iterate over tab actors and

Something was cut off here, but I understand your concern with tab and thread actors being too intimate and I agree that it's not a good outcome either. Maybe I'm being too paranoid about this, but at the very least we should document in a comment somewhere that the event in this case needs to be emitted synchronously, so even if we switch to an async-by-default event emitter library, we will still use the sync one in this case.

I wish I could think of a way to write a test for this, but since it's a race I don't know how to make the thread actor miss a script consistently on reload.

> > Another change from the previous behavior is that we now emit the
> > navigate/will-navigate events before doing the actual work, instead of
> > after. Why is that? I'm not sure if it breaks anything currently (there are
> > hardly any subscribers to these events), but it doesn't make sense to me
> > anyway: "load" events are dispatched after the page loads, not before.
> 
> I'm not convinved it makes any difference as the order of execution of
> server vs client is going to be hardly shuffled by the network. In most
> cases the server code will do stuff on willNavigate/navigate before the
> client receive the related debugger protocol packets. I think it is sane to
> ensure replicating that order so that we do not get differences between
> local and remote connections.
> Also I do think it is usefull to handle the event on the server *before* the
> client actually receive the related event. Like switching to a new root node
> in the inspector... Otherwise the client may race the server while it is
> still processing the event he just sent.

Don't forget that in the local debugging case no network is involved and everything devolves into direct method calls. But even so, I think you are right here. Making the thread actor's work trigger on the event pretty much requires the event to fire before sending the response.

> Hum, I'm not seing this behavior. It stops on the breakpoint just after the
> tabNavigated:start. Then when I resume, I get the tabNavigated:stop. I
> tested on Firefox against a tab.
> Does it fire two tabNavigated event before we hit the breakpoint?

Yes, that's what I saw. I'll try it again with your updated patch.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 53 • 10 years ago

In bug 983386 comment 17 I explained the history behind the various state transitions we are monitoring, but I'll copy it here for convenience:

It's not that surprising that these 3 tools (console, debugger, inspector) rely on different state changes. The debugger, which was the first, was only interested at the moment before navigation started and then after the window global was created, hence its reliance on isWindow. The console, which came next, was interested in network events, so that's where the isNetwork came from. The inspector on the other hand deals with a document, so it needs to watch for isDocument state changes mostly.

The still open question is whether we can unify the code handling all those state changes with the minimum number of checks. I wish there was a document or a state diagram that explained all the state transitions, but people were giggling last time I asked for one. Your patch however makes me hopeful that there is a small common subset of state changes that we can use going forward.

	Alexandre Poirot [:ochameau] Assignee
	Comment 54 • 10 years ago

- Renamed _onWindowReady/_onWillNavigate/_onNavigate to _windowReady/_willNavigate/_navigate
- Used only one argument and destructuring for (will-)navigate events
- Moved dumpn before emitting events (having them dumped is handy, should I really drop them before landing?)

I still would like to somehow write a test that would verify the expectations
we makes for each tools regarding navigate and willNavigate.
Or at the very least, document in _willNavigate/_navigate what are these expectations.

Let me summarize these events:

- will-navigate shouldn't change.

  `isWindow && isDocument && isStart` is only true for one single call of onStateChange.
  (At least in all the various cases I tested.)
  (So checking isWindow && isStart or isDocument && isStart won't change much)

- navigate didn't changed!

  I said in comment 50 we changed from `isStop && isWindow` to `isStop && isDocument`,
  but that's false. TabActor and Inspector were already using isStop && isWindow:
  # inspector.js:
  if (isWindow && (flags & Ci.nsIWebProgressListener.STATE_STOP)) {
    events.emit(this, "windowchange-stop", progress.DOMWindow);
  }

  # webbrowser.js:
  // Skip non-interesting states.
  if (!isWindow || !isNetwork ||
    aProgress.DOMWindow != this._tabActor.window) {
    return;
  }
  ...
  } else if (isStop) {
    ...
    this.emit("navigate", packet);
  }

I'll attach a debug patch with trace output right after that give more information about onStateChange calls.
But I'm pretty confident navigate and `isWindow && isStop` is equivalent of load event,
one of the last event we can listen too. And tbh, I'm not sure we want to wait for this event,
as it may block our tools until all ressources are loaded :/

	Alexandre Poirot [:ochameau] Assignee
	Comment 55 • 10 years ago

Here is a patch to debug onStateChange calls.

Here is the traces I got.

Session restore google.fr:
start: 1 stop:0 document:131072 window:524288 network:262144 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:interactive 
start: 0 stop:16 document:0 window:524288 network:262144 readyState:complete 

Reload google.fr:
start: 1 stop:0 document:131072 window:524288 network:262144 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:interactive 
start: 0 stop:16 document:0 window:524288 network:262144 readyState:complete 

load lemonde.fr (lot of iframes):
start: 1 stop:0 document:131072 window:524288 network:262144 readyState:complete 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:uninitialized 
start: 0 stop:16 document:0 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:uninitialized 
start: 0 stop:16 document:0 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:interactive 
start: 0 stop:16 document:0 window:524288 network:262144 readyState:complete 
start: 1 stop:0 document:131072 window:524288 network:262144 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:262144 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:uninitialized 
start: 0 stop:16 document:0 window:524288 network:262144 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:262144 readyState:complete 

back to google.fr:
start: 1 stop:0 document:131072 window:524288 network:262144 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:262144 readyState:complete 

forward to lemonde.fr:
start: 1 stop:0 document:131072 window:524288 network:262144 readyState:complete 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:uninitialized 
start: 0 stop:16 document:0 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:uninitialized 
start: 0 stop:16 document:0 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:uninitialized 
start: 0 stop:16 document:0 window:524288 network:0 readyState:uninitialized 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 1 stop:0 document:131072 window:524288 network:0 readyState:uninitialized 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:complete 
start: 0 stop:16 document:0 window:524288 network:0 readyState:complete 
start: 0 stop:16 document:131072 window:0 network:0 readyState:interactive 
start: 0 stop:16 document:0 window:524288 network:262144 readyState:complete 

We can see that:
 * isDocument, isWindow and isNetwork are always all true on isStart,
   but only for top level document. For iframe isNetwork will always be false.
 * readyState being always interactive when isStop and isDocument,
   and it is complete when we get isStop and isWindow.
 * isNetwork is always true when isStop and isWindow are true, but only for the
   top level document.

So it seems fine to drop isNetwork, as we want the start and stop events for iframes too,
and for top level it looks always true.
And isDocument and isWindow are always true in the same call when we get isStart.

	Alexandre Poirot [:ochameau] Assignee
	Comment 56 • 10 years ago

So I think I didn't changed much with this patch, but my comment about "are we sure about what we are waiting for" still applies!

Mihai gave us a good description of his expectations for the console code in comment 51.
Given his description, will-navigate looks good, we may be able to add some specific assertion for this. But regarding navigate, we fire ways after his particular needs. document-element-inserted may be a better match (on top of being more precisely defined than the onStateChange call).

But it is still vague for the debugger. I'm not sure the debugger even cares about navigate. I tend to think we should be doing everything on DOMWindomCreated, which seems to better match debugger needs.
That's why I started to mix/merge in my first patches stuff from navigate and DOMWindowCreated.

The definition you gave in comment 53 is a combination of "start or stop" and "document, window or network". That is very vague and even with the few tests I just made, I can't describe exactly when onStateChange is really called. DOMWindowCreated, DOMContentLoaded, pagehide, pageshow or load events are very precisely defined. Some other platform events like document-element-inserted, dom-window-destroyed, dom-window-frozen are also more explicit.

At the end, we can keep it "as-is", but this piece of darkness if our code may hide a bunch of slowness (by using an event that comes last) or a bunch of races (if the event doesn't ensure all expectations).

	Alexandre Poirot [:ochameau] Assignee
	Comment 57 • 10 years ago

See previous comments and the interdiff!
(I still need to put a comment for the synchronous event emitter assumption)

Also if you see something broken, give it a try on nightly,
I've seen many things being broken on nightly.
(for ex: about:app-manager doesn't have resources in debugger, quick back and forth mixes resources in the debugger,
back and forth tends to easily throw exception in the inspector)

	Alexandre Poirot [:ochameau] Assignee
	Comment 58 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=724b9b689fe5

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 59 • 10 years ago

Comment on attachment 8406290 [details] [diff] [review]
Use same ProgressListener event across actors

Review of attachment 8406290 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like you have a bug to fix when child processes are used, but other than that this looks good to me. I couldn't reproduce the out of order packets I saw in the past. If you feel strongly about keeping the dump calls, then OK. It's just that we've been adding more and more, and not all of them have added significant value. Also if you were using the devtools event emitter implementation, it does its own logging on emit() guarded by another preference setting.

Don't forget to add the sync event emitter comment, too.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +524,5 @@
>     * An object on which listen for DOMWindowCreated and pageshow events.
>     */
>    get chromeEventHandler() {
> +    // TODO: bug 992778, fix docShell.chromeEventHandler in child processes
> +    return this.docShell.chromeEventHandler ||

This seems to be the cause of the mochitest breakage. It causes an infinite recursion with the docshell definition in childtab.js.

@@ +922,5 @@
> +      window: window,
> +      isTopLevel: isTopLevel
> +    });
> +
> +    // TODO: move that code to ThreadActor by listening to window-ready

Please file a bug and add the bug number to the TODO comments.

	Alexandre Poirot [:ochameau] Assignee
	Comment 60 • 10 years ago

Ok, I removed the dumpn(), I'm not used to the event emitted logging helper.
Also added some comment about will-navigate/navigate assumptions/behavior,
feel free to review them!

	Alexandre Poirot [:ochameau] Assignee
	Comment 62 • 10 years ago

As promised, a test to formalize will-navigate/navigate
and at least assert mihai expectation.
If you have any other to add...

But I can't promise it will work on slaves,
as it is full of events assertions :/

https://tbpl.mozilla.org/?tree=Try&rev=9ce3a9e34a75

	Alexandre Poirot [:ochameau] Assignee
	Comment 63 • 10 years ago

New patch (very small modification to ensure we have a running server), new try:
https://tbpl.mozilla.org/?tree=Try&rev=f94336a10d92

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 64 • 10 years ago

> Ok, I removed the dumpn(), I'm not used to the event emitted logging helper.

Note that you need to require("devtools/toolkit/event-emitter"), not the sdk module, to log when devtools.dump.emit is true.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 65 • 10 years ago

Comment on attachment 8407588 [details] [diff] [review]
Add test for will-navigate/navigate/tabNavigate

Review of attachment 8407588 [details] [diff] [review]:
-----------------------------------------------------------------

I like it!

::: toolkit/devtools/server/tests/browser/navigate-first.html
@@ +11,5 @@
> +  e.returnValue="?"
> +};
> +//window.onbeforeunload = function() {
> +//  alert("Don't leave my website!");
> +//}

Don't forget to remove the commented-out code.

	Alexandre Poirot [:ochameau] Assignee
	Comment 66 • 10 years ago

Merged the test into this patch and removed the commented code.

	Alexandre Poirot [:ochameau] Assignee
	Comment 67 • 10 years ago

Comment on attachment 8408432 [details] [diff] [review]
Use same ProgressListener event across actors

https://hg.mozilla.org/integration/fx-team/rev/2957a9bc0e48

	Carsten Book [:Tomcat]
	Comment 68 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/2957a9bc0e48

	Alexandre Poirot [:ochameau] Assignee
	Comment 69 • 10 years ago

Ok, so now, here is the actor patch to support frame listing and switching.

Hacking window-ready/window-destroy/navigate/will-navigate events allows supporting
frame switching for free from the toolbox.
i.e. we don't have to change anything in client/UI/Toolboxes code,
but we have to do with the various subtle expections actors have on these events.
And sometimes, it wasn't enough, like webconsole, where I had to dispatch
a new dedicated event 'changed-toplevel-document' to notify when we are changing the targeted document.

I tried to put various helpful comment whereever something non trivial had to be addressed.

I tested this patch on desktop with yahoo.com and its iframes
(almost all websites have iframes... for ads, trackings, social network widgets, ...).
Testing many combinations of opening, closing, destroying frames.
I also tested on b2g, with content processes special iframes.
You can test it easily with "UI tests" app of gaia, going into window.open section.
Then you click on window.open, it will create a new root docshell within the app process.

Panos, I feel quite bad spamming you with patches,
please do not hesitate to involve anyone else in this review!

https://tbpl.mozilla.org/?tree=Try&rev=c012f0b42462

	Alexandre Poirot [:ochameau] Assignee
	Comment 70 • 10 years ago

And the related client patch that adds the "Frames" button to toolboxes.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 71 • 10 years ago

Comment on attachment 8442014 [details] [diff] [review]
Tweak TabActor to support changing its targeted context to an iframe

Review of attachment 8442014 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, but I'd like Patrick to take a look too, particularly on the webbrowser.js changes that impact the inspector & highlighter.

::: toolkit/devtools/server/actors/webbrowser.js
@@ +741,5 @@
> +    // Ensure replying to attach() request first
> +    // before notifying about new docshells.
> +    Services.tm.currentThread.dispatch(() => {
> +      this._watchDocshells();
> +    }, 0);

Nit: you could make this look more like a lambda by using the condensed form: () => this._watchDocshells()

@@ +767,5 @@
> +    try {
> +      win = Services.wm.getOuterWindowWithId(windowId);
> +    } catch(e) {}
> +    if (!win) {
> +      return { error: "The related docshell is destroyed or not found" };

The "error" field should contain a short value, like "noWindow", for use by computers, and an additional "message" field should contain the human-recognizable string above:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Error_Packets

@@ +773,5 @@
> +
> +    // Reply first before changing the document
> +    Services.tm.currentThread.dispatch(() => {
> +      this._changeTopLevelDocument(win);
> +    }, 0);

<insert same nitpicky comment about lambdas here>

@@ +823,5 @@
> +    let window = webProgress.DOMWindow;
> +    let id = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                   .getInterface(Ci.nsIDOMWindowUtils)
> +                   .outerWindowID;
> +    let parentId = null;

It seems better from a protocol POV to leave parentId undefined here, so in case there is no parent, the packet will not contain the property at all. What do you think?

@@ +843,5 @@
> +  _updateChildDocShells: function () {
> +    let containedDocShells = this.webProgress.getDocShellEnumerator(
> +                               Ci.nsIDocShellTreeItem.typeAll,
> +                               Ci.nsIDocShell.ENUMERATE_FORWARDS);
> +    while (containedDocShells.hasMoreElements()) {

How often is this called? Would it make sense to have the tabDocShellUpdate packet return an array of entries to cut down on protocol traffic?

@@ +863,5 @@
> +                     windowId: id,
> +                     destroy: true
> +                   });
> +
> +    // Stop watching this docshell if that's a root one.

Nit: "if it's a root one"

@@ +1139,5 @@
> +    this._willNavigate(this.window, window.location.href, null, true);
> +
> +    this._windowDestroyed(this.window);
> +
> +    Services.tm.currentThread.dispatch(() => {

Grrr, if we weren't still using sync promises on the server, this would have been a simple promise chain...

Can you add a comment to use promises here once bug 943517 is fixed?

@@ +1156,5 @@
> +                         .getInterface(Ci.nsIWebNavigation)
> +                         .QueryInterface(Ci.nsIDocShell);
> +    // Here is the very important call where we switch the currently
> +    // targeted context. (it will indirectly update this.window and
> +    // many other attributes defined from docShell)

Nit: move the period after the closing paren.

Might also want to use periods consistently at the end of comments if you are suffering from OCD like me :)

@@ +1170,5 @@
>     * Handle location changes, by clearing the previous debuggees and enabling
>     * debugging, which may have been disabled temporarily by the
>     * DebuggerProgressListener.
>     */
> +  _windowReady: function (window, isFakeEvent) {

Using a default false value for isFakeEvent (isFakeEvent = false) doesn't force the API change to every call site.

@@ +1175,4 @@
>      let isTopLevel = window == this.window;
>  
> +    // We just reseted iframe list on WillNavigate, we now list all existing
> +    // frames when we load a new document in the original window

Nit: "We just reset the iframe list on _willNavigate, so we now..."

@@ +1217,3 @@
>     * being loaded in the currently targeted context.
>     */
> +  _willNavigate: function (window, newURI, request, isFakeEvent) {

Same comment about a default value here.

@@ +1705,5 @@
>                                .getInterface(Ci.nsIWebProgress);
> +    try {
> +      webProgress.removeProgressListener(this);
> +    } catch(e) {
> +      Cu.reportError("Exception during removeProgressListener");

Better use reportException() here, from DevToolsUtils.js.

@@ +1818,3 @@
>      let window = aProgress.DOMWindow;
>      if (isDocument && isStart) {
> +      // One of the most early event that tell us a new URI

Nit: "One of the earliest events that tells us..."

::: toolkit/devtools/server/actors/webconsole.js
@@ +1405,5 @@
>      }
>    },
> +
> +  /**
> +   * This listener is called when we switched to another frame,

Typo: "when we switch"

@@ +1406,5 @@
>    },
> +
> +  /**
> +   * This listener is called when we switched to another frame,
> +   * mostly to unregister previous listeners and listen on the new document.

Nit: "and start listening"

@@ +1415,5 @@
> +    let listeners = [...this._listeners];
> +
> +    // Unregister existing listener on the previous document
> +    // (pass a copy of the array as it will shift from it)
> +    this.onStopListeners({listeners: listeners.map((a)=>a)});

listeners.slice() should be faster for copying and makes the intent more clear.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 72 • 10 years ago

Comment on attachment 8442016 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document.

Review of attachment 8442016 [details] [diff] [review]:
-----------------------------------------------------------------

We need to use a different style for the button in the dark theme, because it is illegible as it is. Brian would know how to do that and I'm sure he has an opinion on the toolbox toolbar, so flagging him for an additional review.

Other than that, I only encountered one hang that I mention below that made me not r+ it right away.

::: browser/devtools/framework/target.js
@@ +385,5 @@
>      this.client.addListener("tabNavigated", this._onTabNavigated);
> +
> +    this._onTabDocShellUpdate = function onRemoteTabDocShellUpdate(aType, aPacket) {
> +      this.emit("docshell-update", aPacket);
> +    }.bind(this);

Use an arrow function to avoid the bind() call?

::: browser/devtools/framework/toolbox.js
@@ +1046,5 @@
> +    // We may receive this event before the toolbox is ready,
> +    // in such scenario save this event for later.
> +    if (!this.isReady) {
> +      this._docshells.push(docshell);
> +      return;

I got a hang after some heavy use of the feature while I was in the inspector panel on cnn.com and I wonder if this part was the culprit.

It looks to me that the forEach loop in open() will keep encountering new elements in the array, if we keep pushing them here. Why can't we just ignore the event if the toolbox is not ready?

@@ +1295,5 @@
>        return this._destroyer;
>      }
>  
>      this._target.off("navigate", this._refreshHostTitle);
> +    this._target.off("docshell-update", this._updateDocShell);

Don't we have to nullify this._docshells here too?

::: browser/devtools/framework/toolbox.xul
@@ +72,5 @@
>        <hbox id="toolbox-buttons" pack="end"/>
>        <vbox id="toolbox-controls-separator" class="devtools-separator"/>
> +      <toolbarbutton id="toolbox-docshells" label="&toolboxFramesButton;" type="menu" hidden="true">
> +        <menupopup></menupopup>
> +      </toolbarbutton>

It seems to me that if we'll be using a text button, it would be best to place it between the tabs and the buttons, otherwise it looks kinda lonely between those icons.

I can't think of a good graphical metaphor for the button either, so text may be the best option.

::: browser/devtools/inspector/test/browser_inspector_select_docshell.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +let Toolbox = devtools.Toolbox;

Please add a brief comment in the beginning of the file that explains what this test is about.

	Brian Grinstead [:bgrins]
	Comment 73 • 10 years ago

Comment on attachment 8442016 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document.

Review of attachment 8442016 [details] [diff] [review]:
-----------------------------------------------------------------

This is coming together nicely - here are a few notes for the frontend:

1) The button does look better on the left of the toolbox buttons IMO.  I would actually put it inside of the toolbox-buttons, then change the markup to be something like below.  The padding will be a bit weird for now, but once Bug 1028252 lands that should be resolved.

      <hbox id="toolbox-buttons" pack="end">
      <toolbarbutton class="devtools-toolbarbutton" text-as-image="true" id="toolbox-docshells" label="&toolboxFramesButton;" type="menu" hidden="true">
        <menupopup position="bottomright topright"></menupopup>
      </toolbarbutton>
      </hbox>

2) The currently selected frame should be marked as checked when the popup is opened

3) This should be hideable in the options panel under the 'Available Toolbox' list (probably at the top of that checkbox list since that matches the visible order)

4) It seems like there should be some kind of indication when you have a non-root frame selected.  The easiest way to do this would be to add the "checked=true" attribute to the toolbarbutton, which will make it kind of blueish / selected.

5) I randomly had this page opened and switching between frame breaks the inspector: chrome://browser/content/devtools/projecteditor-loader.xul

	Patrick Brosset <:pbro>
	Comment 74 • 10 years ago

Comment on attachment 8442014 [details] [diff] [review]
Tweak TabActor to support changing its targeted context to an iframe

Review of attachment 8442014 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see any problem related to the highlighter here, it seems to work fine. Of course it's still possible to pick elements from outside the currently selected frame, but that has no effect on the inspector and doesn't throw any exceptions, so we're fine here.

The inspector seems to work too, however it does throw a stacktrace to the logs when switching to an iframe (tested on jsbin.com).
The stacktrace comes from this line: http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2416 and is related to how the inspector tracks mutations.
The way it works is the WalkerActor sends a protocol.js event to the client when a DOM mutation occurs, then the WalkerFront, upon receiving the event, calls the actor to get the list of queued mutations.
Looking at TabActor._changeTopLevelDocument, I don't really see why this would fail yet.
This only seems to happen when you switch from the root frame to a child frame, not the other way around.
The problem seems to come from http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js#2033. frameActor is normally undefined either when the root document loads (in which 'frame' is null and therefore frameActor too), or when the document contains an iframe that loads but that hasn't been seen by the walker yet.
In case of frame switching, even if the new frame is considered as topLevel, 'frame' is defined, and is known in the refMap, so frameActor is defined, and we end up queuing a 'frameLoad' event, which the front-end doesn't like.

Also, there should be a check for the current selected frame so that no events are sent if you switch the same frame (right now, the inspector reloads even if you select the same frame again).

Other than this, it's pretty awesome! It helps a lot.
Testing on jsbin.com (which I think is a pretty good testcase for this), it handles frames deletion nicely. As Brian said, we need a way to know which frame is currently selected in the list however, it's easy to get lost otherwise.

	Joe Walker [:jwalker]
	Comment 75 • 10 years ago

I don't think we should have the work 'Frame' in the toolbox toolbar. We're cramped for space as it is, and this is a fairly advanced feature. We could imagine that UI elements should occupy a space somewhat relative to their frequency of use. The current UI for this feature is 3-4x the size of the highlighter button, but probably used at least an order of magnitude less.

So I think we should have a icon, perhaps one that looks like a frame with a bottom-right corner arrow to indicate a sub-menu.

	Paul Rouget [:paul]
	Comment 76 • 10 years ago

How does it work with Chrome/Safari/Firebug?

	Girish Sharma [:Optimizer]
	Comment 77 • 10 years ago

Chrome only shows the frame selection dropdown in their Console tool

	Alexandre Poirot [:ochameau] Assignee
	Comment 79 • 10 years ago

So I modified many things, tried to address all review comments.
* I ended up modifying message names and various function name to
use frame instead of docshell.
* _updateChildDocShells now send multiple docshell updates at once
* chrome://browser/content/devtools/projecteditor-loader.xul now works.
* I updated the frames buttons as Brian described, I'm open to any other proposal
as soon as it doesn't block landing this patch (server side/actor one)!

	Alexandre Poirot [:ochameau] Assignee
	Comment 80 • 10 years ago

UI patch, subject to change given UX feedback.
Joe, can you see if darrin, or anyone else, can finally spend some cycles on this?

Also, for both patches, please refer to attachment 8446602 [details] [diff] [review] for interdiff.

	Alexandre Poirot [:ochameau] Assignee
	Comment 81 • 10 years ago

Comment on attachment 8446610 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document.

Review of attachment 8446610 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/inspector/inspector-panel.js
@@ +210,5 @@
>      return walker.getRootNode().then(aRootNode => {
>        rootNode = aRootNode;
> +      if (this.selectionCssSelector) {
> +        return walker.querySelector(rootNode, this.selectionCssSelector);
> +      }

That modification fixed projecteditor-loader.xul support...
I can't explain why it wasn't failing before this patch.
selectionCssSelector shouldn't be defined here and should always make querySelector throw.

	Alexandre Poirot [:ochameau] Assignee
	Comment 82 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=8e9e9f2fc13c
I still have a test failure, about the added command-button,
but that should be easy to fix.

Thanks Patrick for comment 74, it is really helpful.
I think I fixed that in by only queuing 'newRoot' in inspector.js:onFrameLoad
when we are processing a top-level window.
I no longer see getMutations exceptions.

	Brian Grinstead [:bgrins]
	Comment 83 • 10 years ago

Comment on attachment 8446610 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document.

Review of attachment 8446610 [details] [diff] [review]:
-----------------------------------------------------------------

I have some concerns about these patches with the network panel selected:

1) Open https://bgrins.github.io/devtools-demos/inspector/iframe.html.
2) Switch to network panel.   Note: it can be helpful to do 'disable cache' at this point to make things a bit less confusing.
3) Switch to one of the secondary frames
4) Press the 'reload' button.  Nothing happens on page reload and button is still there.
5) Press the 'performance analysis' button.  It seems to be empty.

Even if the initial listing comes through when reloading on the main frame, there is some general weirdness on that page with the network monitor when switching between frames and reloading.  I suspect there is also issues with canvas debugger / shader editor / profiler but I haven't tested yet.

I also once got a hang when quickly switching between frames on that demo page with the inspector open.  Unfortunately I have not been able to reproduce since

::: browser/devtools/framework/toolbox.js
@@ +1084,5 @@
> +        menu.setAttribute("checked", "true");
> +      } else {
> +        menu.removeAttribute("checked");
> +      }
> +      // Uncheck the previously selected frame

The frontend is looking better, but I'm still not seeing the currently selected frame as being checked in the UI when opening the popup

	Patrick Brosset <:pbro>
	Comment 84 • 10 years ago

Comment on attachment 8446644 [details] [diff] [review]
Tweak TabActor to support changing its targeted context to an iframe

Review of attachment 8446644 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still happy with this patch (note that my review is limited to inspector.js and webbrowser.js, I let Panos re-review the rest).
The fix to suppress the stacktrace I was seeing seems ok to me.

::: toolkit/devtools/server/actors/inspector.js
@@ +2026,5 @@
>        this.queueMutation({
>          type: "newRoot",
>          target: this.rootNode.form()
>        });
> +      return;

Yeah, that should work indeed. In any case, without your patch, we were returning at line 2035, just below, that that doesn't change the normal usecase.

	Joe Walker [:jwalker]
	Comment 85 • 10 years ago

Comment on attachment 8446610 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document.

Review of attachment 8446610 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd
@@ +20,5 @@
>  <!ENTITY toolboxReload.key             "r">
> +<!-- LOCALIZATION NOTE (toolboxFramesButton): This is the label for
> +  -  the iframes menu list that appears only when the document has some.
> +  -  It allows you to switch the context of the whole toolbox. -->
> +<!ENTITY toolboxFramesButton           "Frames">

I'd like to make sure comment 75 doesn't get lost. I think we should have an icon for this rather than a text button.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 86 • 10 years ago

Comment on attachment 8446644 [details] [diff] [review]
Tweak TabActor to support changing its targeted context to an iframe

Review of attachment 8446644 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/webbrowser.js
@@ +771,5 @@
> +      return { error: "noWindow",
> +               message: "The related docshell is destroyed or not found" };
> +    } else if (win == this.window) {
> +      return { error: "sameWindow",
> +               message: "You are already targetting this window" };

What harm would it do to just return successfully here without doing anything (i.e. just "return {};")? Remember Postel's law: "be conservative in what you do, be liberal in what you accept from others".

@@ +1775,5 @@
>        return;
>      }
>  
>      let window = evt.target.defaultView;
> +    this._tabActor._windowReady(window, false);

This is not needed, since false is the default value.

@@ +1841,2 @@
>        let newURI = aRequest instanceof Ci.nsIChannel ? aRequest.URI.spec : null;
> +      this._tabActor._willNavigate(window, newURI, aRequest, false);

Same here, false is the default.

	Brian Grinstead [:bgrins]
	Comment 87 • 10 years ago

Comment on attachment 8446610 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document.

Review of attachment 8446610 [details] [diff] [review]:
-----------------------------------------------------------------

A few more notes, along with one other thing I've noticed - the button doesn't show up in the Browser Toolbox (although it does seem like the button is there in the DOM, I don't see the 'frames' text even).

::: browser/app/profile/firefox.js
@@ +1248,5 @@
>  pref("devtools.toolbox.zoomValue", "1");
>  
>  // Toolbox Button preferences
>  pref("devtools.command-button-pick.enabled", true);
> +pref("devtools.command-button-frames.enabled", true);

I'm guessing this should be false by default

::: browser/devtools/framework/toolbox.js
@@ +49,5 @@
> +// addons that have manually inserted toolbarbuttons into DOM.
> +// (By default, supported target is only local tab)
> +const ToolboxButtons = [
> +  { id: "command-button-frames",
> +    isTargetSupported: target => (!target.isAddon && target.activeTab && target.activeTab.traits.frames) },

Please move this below command-button-pick so it matches the order they would show up in the toolbox

@@ +570,4 @@
>     * Add buttons to the UI as specified in the devtools.toolbox.toolbarSpec pref
>     */
>    _buildButtons: function() {
>      if (!this.target.isAddon) {

Does this check still need to be here with the new isTargetSupported functionality in ToolboxButtons?  I believe the picker button will be hidden if the target is an addon.

@@ +1084,5 @@
> +        menu.setAttribute("checked", "true");
> +      } else {
> +        menu.removeAttribute("checked");
> +      }
> +      // Uncheck the previously selected frame

The frontend is looking better, but I'm still not seeing the currently selected frame as being checked in the UI when opening the popup

::: browser/devtools/framework/toolbox.xul
@@ +69,5 @@
>      <toolbar class="devtools-tabbar">
>        <hbox id="toolbox-picker-container" />
>        <hbox id="toolbox-tabs" flex="1" />
> +      <hbox id="toolbox-buttons" pack="end">
> +        <toolbarbutton class="devtools-toolbarbutton" text-as-image="true" id="command-button-frames" label="&toolboxFramesButton;" type="menu" hidden="true">

I don't see any label next to the frames button - you need to set the tooltiptext attribute on the button to whatever you want to show up in the options panel - Maybe "Select an iframe as the main window"?  I don't like that string, but we could come up with something better.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 88 • 10 years ago

Comment on attachment 8446644 [details] [diff] [review]
Tweak TabActor to support changing its targeted context to an iframe

Review of attachment 8446644 [details] [diff] [review]:
-----------------------------------------------------------------

One thought occurred to me while reviewing the UI patch: there is no "listFrames" request. It seems as you feel it won't be necessary, because it would only be used once on tool startup, just like "listSources". However the fact of the matter is that without it there is a potential race between toolbox initialization and new frame events. The hangs we've been observing could be related and I've also mentioned it in comment 72.

It seems to me that adding a "listFrames" request would make the frontend code much easier to follow and reason about.

::: toolkit/devtools/client/dbg-client.jsm
@@ +221,5 @@
>    "tabListChanged": "tabListChanged",
>    "reflowActivity": "reflowActivity",
>    "addonListChanged": "addonListChanged",
>    "tabNavigated": "tabNavigated",
> +  "framesUpdate": "framesUpdate",

And another thing: "frameUpdate" would be more appropriate I think.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 89 • 10 years ago

Comment on attachment 8446610 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document.

Review of attachment 8446610 [details] [diff] [review]:
-----------------------------------------------------------------

Besides the need for a "listFrames" protocol request, the main problems I can see is the checkbox that Brian mentioned, the missing entry in the options panel that the test failure highlighted and the icon that Joe mentioned. I actually like that icon idea a lot, it can be a riff on the responsive mode icon with the frame and a small arrow.

::: browser/devtools/framework/toolbox.js
@@ +259,5 @@
>  
>        let domReady = () => {
>          this.isReady = true;
> +        // Flush any event received while the toolbox was still loading...
> +        this._framesEvents.forEach(data => this._updateFrames(null, data));

So this covers any events received between this._target.makeRemote() below and DOMContentLoaded? But what about any frames present in the page before the toolbox was opened?

Sending a "listFrames" request right after we add the frames-update listener seems like a more safe approach.

@@ +1063,5 @@
> +    // in such scenario save this event for later.
> +    if (!this.isReady) {
> +      this._framesEvents.push(data);
> +      return;
> +    }

I still don't like this bit, see comment 72 and comment 88.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 90 • 10 years ago

Darrin could you produce an icon along the lines of Joe's idea in comment 75? Perhaps a riff on the responsive mode icon.

	Brian Grinstead [:bgrins]
	Comment 91 • 10 years ago

Comment on attachment 8446644 [details] [diff] [review]
Tweak TabActor to support changing its targeted context to an iframe

Review of attachment 8446644 [details] [diff] [review]:
-----------------------------------------------------------------

Panos' point about about a potential race could indeed be what is causing the random hangs I've seen

	Alexandre Poirot [:ochameau] Assignee
	Comment 92 • 10 years ago

New set of patches. This time I don't get interdiff, instead,
I split the patch in multiple ones.

I tried to fix support of other tools like webgl and webaudio.
But we shouldn't hold landing these patches if one particular tool fails.
This patch is already large enough. We should open some followups to tweak each tool one by one.

https://tbpl.mozilla.org/?tree=Try&rev=d3650bb1ef63

	Alexandre Poirot [:ochameau] Assignee
	Comment 93 • 10 years ago

I finally introduced a `listFrames` request and use that instead during startup.
The button behavior and label should be correct now.

	Alexandre Poirot [:ochameau] Assignee
	Comment 94 • 10 years ago

The webconsole needs some tweaks to work properly when switching frames.
We need to unregister various listeners being set on the previous frame
and register new ones to the selected frame.

	Alexandre Poirot [:ochameau] Assignee
	Comment 95 • 10 years ago

I think you already r+ this part, but just to be sure.
It also contains the test for the whole frame selection feature.

	Alexandre Poirot [:ochameau] Assignee
	Comment 96 • 10 years ago

This patch mostly replace ContentObserver usage by listening to window-ready/window-destroyed
events being dispatched on the tab actor. So that it can automagically work with frames
by receiving these event when switching to another frame.
Also there is various `if (isFakeEvent) {}`; that's to prevent hiding the Reload button
when switching to another frame.
When we are switching to a frame, we have to reload it to make these tools to work...

	Alexandre Poirot [:ochameau] Assignee
	Comment 97 • 10 years ago

Finally, some random fixes to make the netmonitor to work with frames.
See inlined comments.

	Mihai Sucan [:msucan]
	Comment 98 • 10 years ago

Comment on attachment 8468468 [details] [diff] [review]
Tweak webconsole for frames selection.

Review of attachment 8468468 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

	Mihai Sucan [:msucan]
	Comment 99 • 10 years ago

Comment on attachment 8468475 [details] [diff] [review]
Tweak netmonitor for frames selection patch

Review of attachment 8468475 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks!

	Alexandre Poirot [:ochameau] Assignee
	Comment 100 • 10 years ago

Fix additional TBPL errors.

	Alexandre Poirot [:ochameau] Assignee
	Comment 101 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=005a4b265da5

	Patrick Brosset <:pbro>
	Comment 102 • 10 years ago

Comment on attachment 8469623 [details] [diff] [review]
Fix inspector and add test for frame selection

Review of attachment 8469623 [details] [diff] [review]:
-----------------------------------------------------------------

R+ this simple patch, but I'd really like to see the new test being rewritten to fit with the rest.

::: browser/devtools/inspector/test/browser.ini
@@ +55,5 @@
>  [browser_inspector_search-navigation.js]
>  [browser_inspector_sidebarstate.js]
>  [browser_inspector_switch-to-inspector-on-pick.js]
>  [browser_inspector_update-on-navigation.js]
> +[browser_inspector_select_docshell.js]

Please keep this file sorted in alphabetically order.

::: browser/devtools/inspector/test/browser_inspector_select_docshell.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

General comment about this test:
We recently refactored/cleaned-up all of the inspector tests (see bug 988314) so that they are all formatted the same, use the same helper functions, etc...
This is the only test that diverges from this now global coding style. Since this is a new test and not an existing one, it would be good to rewrite it so it fits with the rest of the inspector tests codebase.
Also, I believe this would make the test a lot shorter:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

"use strict";

// Test frame selection switching at toolbox level
// when using the inspector

let test = asyncTest(function*() {
  const FrameURL = "data:text/html;charset=UTF-8," + encodeURI("<div id=\"frame\">frame</div>");
  const URL = "data:text/html;charset=UTF-8," + encodeURI("<iframe src=\"" + FrameURL + "\"></iframe><div id=\"top\">top</div>");

  yield addTab(URL);
  let {toolbox, inspector} = yield openInspector();

  info("Verify we are on the top level document");
  ...

  info("Listen to will-navigate to check if the view is empty");
  ...

  info("Navigation to the iframe has started, the inspector should be empty");
  ...

  info("Navigation to the iframe was done, the inspector should be back up");
  ...

  gBrowser.removeCurrentTab();
  Services.prefs.clearUserPref("devtools.command-button-frames.enabled");
});

@@ +6,5 @@
> +
> +let Toolbox = devtools.Toolbox;
> +
> +function test() {
> +  waitForExplicitFinish();

This is not needed, already called in head.js

	Brian Grinstead [:bgrins]
	Comment 103 • 10 years ago

Comment on attachment 8468467 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document

Review of attachment 8468467 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, seems to be much more stable.  A few notes:

1) The browser toolbox seems to break after applying this patch (no tabs or content show up, just an empty window).
2) We are still waiting on an icon for the command button
3) I wonder if there is some additional way we can indicate which frame is selected to prevent any confusion - maybe an always visible popup above the frame button that shows the URL, or maybe hovering the command button could draw the box model highlighter on the selected frame?  Nothing I would block this landing with, but just throwing out a couple of ideas.

::: browser/devtools/framework/target.js
@@ +369,5 @@
>        let event = Object.create(null);
>        event.url = aPacket.url;
>        event.title = aPacket.title;
>        event.nativeConsoleAPI = aPacket.nativeConsoleAPI;
> +      event.isFakeEvent = aPacket.isFakeEvent;

What is the purpose of isFakeEvent?  I don't see it used anywhere else in this patch

::: browser/devtools/framework/toolbox.xul
@@ +69,5 @@
>      <toolbar class="devtools-tabbar">
>        <hbox id="toolbox-picker-container" />
>        <hbox id="toolbox-tabs" flex="1" role="tablist" />
> +      <hbox id="toolbox-buttons" pack="end">
> +        <toolbarbutton class="devtools-toolbarbutton" text-as-image="true" id="command-button-frames"

Please add a newline for each attribute and line them up with the first one (see other elements in this file).

	Alexandre Poirot [:ochameau] Assignee
	Comment 104 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #103)
> Comment on attachment 8468467 [details] [diff] [review]
> 
> 1) The browser toolbox seems to break after applying this patch (no tabs or
> content show up, just an empty window).

Fixed it, it was because of the call to listFrames in toolbox.js.
The browser toolbox's target doesn't have form.actor attribute.
I addressed this issue for Addon toolbox, but not for the browser one.

> 2) We are still waiting on an icon for the command button

I'll see if we can get some traction...
You really don't want me to provide an icon :p
If I don't get icons by the time Panos get back, I'll land these patches and only hold the very last UI bits.

> 3) I wonder if there is some additional way we can indicate which frame is
> selected to prevent any confusion -
> maybe an always visible popup above the frame button that shows the URL,

Note that the frames button should be in a special state when you select a frame.
On my linux it switches to a blue background.

> or maybe hovering the command button could draw the box model highlighter 
> on the selected frame? 

Can we instanciate an highlighter without instanciating the inspector?
If yes, that sounds like a great way to identify visible iframes,
but doesn't help with all the invisibles one :/

> 
> ::: browser/devtools/framework/target.js
> @@ +369,5 @@
> >        let event = Object.create(null);
> >        event.url = aPacket.url;
> >        event.title = aPacket.title;
> >        event.nativeConsoleAPI = aPacket.nativeConsoleAPI;
> > +      event.isFakeEvent = aPacket.isFakeEvent;
> 
> What is the purpose of isFakeEvent?  I don't see it used anywhere else in
> this patch

This is used in attachment 8468473 [details] [diff] [review], in order to detect event being faked by frame switching.
The main reason to have such flag is to be able to know if the related document
is brand new (non fake event) and we are very early in document loading process.
Or if we are switching to an iframe, in such case, the document already exists for quite a bit.
It allows to know in webgl and webaudio actors, if we have to reload the document (we have to reload for fake event/iframes),
or if we are early enough in load process to be able to override webgl/webaudio API correctly.

	Alexandre Poirot [:ochameau] Assignee
	Comment 105 • 10 years ago

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #102)
> Comment on attachment 8469623 [details] [diff] [review]
> R+ this simple patch, but I'd really like to see the new test being
> rewritten to fit with the rest.

Thanks for the test template, the test is now much more simple!
(Do not hesitate to suggest more tweaks)
Try is still green:
  https://tbpl.mozilla.org/?tree=Try&rev=999224ea17ed
But I'm seeing some intermittent timeout when running it locally.
It seems to only fail when I'm running only it.
It is stuck on "INFO Waiting for the inspector to update",
but the inspector looks already updated...

	Victor Porof [:vporof][:vp]
	Comment 106 • 10 years ago

Comment on attachment 8468473 [details] [diff] [review]
Tweak webgl and webaudio actors for frames selection.

Review of attachment 8468473 [details] [diff] [review]:
-----------------------------------------------------------------

Ask me again for review after addressing all comments.

::: browser/devtools/shadereditor/shadereditor.js
@@ +124,5 @@
>      switch (event) {
>        case "will-navigate": {
>          Task.spawn(function() {
>            // Make sure the backend is prepared to handle WebGL contexts.
> +          if (!isFakeEvent) {

I don't fully understand what isFakeEvent represents, and neither would anyone else that's reading any code that's using it. I would definitely recommend naming it differently, and adding comments describing it.

@@ +132,5 @@
>            // Reset UI.
>            ShadersListView.empty();
> +          if (isFakeEvent) {
> +            $("#reload-notice").hidden = false;
> +            $("#waiting-notice").hidden = true;

Please add a comment here too, like you did for webaudioeditor. Also, this needs to be tested.

::: browser/devtools/webaudioeditor/webaudioeditor-controller.js
@@ -200,5 @@
>     * for an audio context notice.
>     */
>    reset: function () {
> -    $("#reload-notice").hidden = true;
> -    $("#waiting-notice").hidden = false;

Are these now handled in _onTabNavigated?

@@ +246,5 @@
> +          // so we don't need to reload anymore and should receive
> +          // new node events.
> +          $("#reload-notice").hidden = true;
> +          $("#waiting-notice").hidden = false;
> +        }

Testme.

::: toolkit/devtools/server/actors/call-watcher.js
@@ -289,5 @@
>      this._tracedGlobals = tracedGlobals || [];
>      this._tracedFunctions = tracedFunctions || [];
>      this._holdWeak = !!holdWeak;
>      this._storeCalls = !!storeCalls;
> -    this._contentObserver = new ContentObserver(this.tabActor);

Was the ContentObserver assimilated by the tab actor? If so, where? and I need to review that. There are a lot of assumptions here about when _onGlobalCreated and _onGlobalDestroyed are invoked.

@@ +381,3 @@
>      let self = this;
>  
> +    if (!isTopLevel) {

Please add a comment here to bug 981748.

@@ +382,5 @@
>  
> +    if (!isTopLevel) {
> +      return;
> +    }
> +    this._tracedWindowId = id;

Is this the same inner window id?

::: toolkit/devtools/server/actors/webaudio.js
@@ -334,5 @@
>      });
> -    // Bind to the `global-destroyed` event on the content observer so we can
> -    // unbind events between the global destruction and the `finalize` cleanup
> -    // method on the actor.
> -    // TODO expose these events on CallWatcherActor itself, bug 1021321

I guess you can now wontfix that bug?

::: toolkit/devtools/server/actors/webgl.js
@@ +391,5 @@
> +    if (isTopLevel) {
> +      removeFromArray(this._programActorsCache, e => e.ownerWindow == id);
> +      this._webglObserver.unregisterContextsForWindow(id);
> +      events.emit(this, "global-destroyed", id);
> +    }

Do all tests still pass?

	Paul Rouget [:paul]
	Comment 107 • 10 years ago

Darrin: ping!

	Dave Camp (:dcamp)
	Comment 108 • 10 years ago

Adding shorlander to the list of UX requests, but I'd also be ok with the engineering folks involved just making a call if UX doesn't have time to weigh in.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 109 • 10 years ago

Also, to help us all evaluate this can someone link to a recent try push or attach a screenshot of the patch?

	Alexandre Poirot [:ochameau] Assignee
	Comment 110 • 10 years ago

(In reply to Jeff Griffiths (:canuckistani) from comment #109)
> Also, to help us all evaluate this can someone link to a recent try push or
> attach a screenshot of the patch?

https://tbpl.mozilla.org/?tree=Try&rev=999224ea17ed
And here is an updated screenshot for the frames button.
It is with a blue background as I select an iframe, otherwise it has a transparent background when you are on the top level document.
See attachment 8389334 [details] for the dropdown menu with iframe list.

	Alexandre Poirot [:ochameau] Assignee
	Comment 111 • 10 years ago

Renamed isFakeEvent to isFrameSwitching

	Alexandre Poirot [:ochameau] Assignee
	Comment 112 • 10 years ago

Same.

	Alexandre Poirot [:ochameau] Assignee
	Comment 113 • 10 years ago

Comment on attachment 8468473 [details] [diff] [review]
Tweak webgl and webaudio actors for frames selection.

I'm moving this patch as a followup in bug 1053805.
We can land support for frame switching for all default tools and then fix all others.

	Paul Rouget [:paul]
	Comment 115 • 10 years ago

Did my best. Use it or not. Up to you.

	Paul Rouget [:paul]
	Comment 116 • 10 years ago

better downscale quality.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 117 • 10 years ago

Comment on attachment 8473060 [details] [diff] [review]
Tweak TabActor to support changing its targeted context to an iframe.

Review of attachment 8473060 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/webbrowser.js
@@ +849,5 @@
> +  },
> +
> +  onListFrames: function BTA_onListFrames(aRequest) {
> +    let windows = this._docShellsToWindows(this.docShells);
> +    return { windows: windows };

It seems a bit confusing to ask for a list of frames and get back a list of windows. listTabs returns a list of tabs as expected, even though the actual protocol forms are pretty much the same.

How about we call the response property "frames" and we change the protocol spec to present both forms as corresponding to DOM windows?

@@ +1218,5 @@
> +    this._willNavigate(this.window, window.location.href, null, true);
> +
> +    this._windowDestroyed(this.window);
> +
> +    Services.tm.currentThread.dispatch(() => {

For future reference, we are replacing most runnable dispatches in server code with DevToolsUtils.executeSoon, which does the right thing in workers. In this case the code shouldn't ever be reached in workers, so using executeSoon is up to you.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 118 • 10 years ago

Comment on attachment 8473061 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document

Review of attachment 8473061 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I'd suggest replacing the label with Paul's icon and having Brian give it a final look.

::: browser/devtools/framework/test/browser_toolbox_options_disable_buttons.js
@@ +62,5 @@
>  
>    for (let tool of toggleableTools) {
>      let isVisible = getBoolPref(tool.visibilityswitch);
>  
> +    let button = toolboxButtonNodes.filter(button=> button.id === tool.id)[0];

Since you agree that some whitespace is needed here, why not add it on both sides of the arrow? :)

::: browser/devtools/framework/toolbox.js
@@ +698,5 @@
>     * preference value.  Simply hide buttons that are preffed off.
>     */
>    setToolboxButtonsVisibility: function() {
>      this.toolboxButtons.forEach(buttonSpec => {
> +      let {visibilityswitch, id, button, isTargetSupported}=buttonSpec;

Nit: whitespace around the assignment please.

::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd
@@ +20,5 @@
>  <!ENTITY toolboxReload.key             "r">
> +<!-- LOCALIZATION NOTE (toolboxFramesButton): This is the label for
> +  -  the iframes menu list that appears only when the document has some.
> +  -  It allows you to switch the context of the whole toolbox. -->
> +<!ENTITY toolboxFramesButton           "Frames">

Paul's icon looks fine to me, so this is no longer necessary.

	Alexandre Poirot [:ochameau] Assignee
	Comment 119 • 10 years ago

Addressed :past comments.

	Alexandre Poirot [:ochameau] Assignee
	Comment 120 • 10 years ago

Hopefully, this is the last review request for the whole patch queue!

	Alexandre Poirot [:ochameau] Assignee
	Comment 121 • 10 years ago

Comment on attachment 8476644 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document

https://tbpl.mozilla.org/?tree=Try&rev=1479d9760e6c

	Alexandre Poirot [:ochameau] Assignee
	Comment 122 • 10 years ago

New try:
https://tbpl.mozilla.org/?tree=Try&rev=e42eaefacfb2

Note that if you want to easily fetch all the patches,
you can also cherry pick them from github:
branch: https://github.com/ochameau/mozilla-central/commits/wip (contains many other patches)
remote: https://github.com/ochameau/mozilla-central.git
git cherry-pick 05a5552055041d5d91e4a0b736b26f4480af0bb0..de68c92650d012e8cb4088065173a50af88c1bdb
(Also contains Bug 1049103, as my patches are based on top of it)

	Tim Nguyen :ntim
	Comment 123 • 10 years ago

Great work ! Although I found a few issues :
- When you have a frame selected (doesn't happen for the top level document), the console won't show the "click to select node in inspector" button for DOM elements output (try document.body for example).

- In the network monitor, if you switch frames, the network timestamps won't be cleared (in the table header). I think the network monitor should be reset to it's initial state (aka with the reload this page notice).

- The switch frames button shows in the Browser Toolbox settings, but doesn't actually show up in the Browser Toolbox (even when the setting is checked).

- On http://smartsearch.altervista.org , the switch frames button has 2 mysterious about:blank frames that I can't switch to

- The switch frames command button doesn't look quite like the other command buttons (mainly because of the different background color)

	Tim Nguyen :ntim
	Comment 124 • 10 years ago

Note that the http://smartsearch.altervista.org bug only happens when you open the toolbox before loading the page.

	Brian Grinstead [:bgrins]
	Comment 125 • 10 years ago

Comment on attachment 8476644 [details] [diff] [review]
Add toolbox-level frame selection to change the currently targeted document

Review of attachment 8476644 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to update the UI with the button on the menu a bit as far as active states and some spacing, but nothing to hold the patch up for (we can do it in a follow-up bug)

::: browser/devtools/framework/toolbox.js
@@ +53,5 @@
> +const ToolboxButtons = [
> +  { id: "command-button-pick",
> +    isTargetSupported: target => !target.isAddon },
> +  { id: "command-button-frames",
> +    isTargetSupported: target => (!target.isAddon && target.activeTab && target.activeTab.traits.frames) },

move the body of the function onto a new line to avoid 80 char limit

	Alexandre Poirot [:ochameau] Assignee
	Comment 126 • 10 years ago

Just fixed the 80 chars limit comment and a test failure.
I don't exactly see what you want to tune to the button,
so please open a followup with precise description or a patch ;)

https://tbpl.mozilla.org/?tree=Try&rev=2c75acd91fef

	Alexandre Poirot [:ochameau] Assignee
	Comment 127 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/ceb3967c0d39
https://hg.mozilla.org/integration/fx-team/rev/f74dd51cbfb9
https://hg.mozilla.org/integration/fx-team/rev/0398f1390ea9
https://hg.mozilla.org/integration/fx-team/rev/c3a7a750a72f
https://hg.mozilla.org/integration/fx-team/rev/f14b2654c180

	Alexandre Poirot [:ochameau] Assignee
	Comment 128 • 10 years ago

(In reply to Tim Nguyen [:ntim] from comment #123)
> Great work ! Although I found a few issues :
> - When you have a frame selected (doesn't happen for the top level
> document), the console won't show the "click to select node in inspector"
> button for DOM elements output (try document.body for example).

It does work here, but I'm seeing issues with the node highlighter
that highlights node at the wrong position. bug 1059312.

> 
> - In the network monitor, if you switch frames, the network timestamps won't
> be cleared (in the table header). I think the network monitor should be
> reset to it's initial state (aka with the reload this page notice).

It reset once you get new data for me.

> 
> - The switch frames button shows in the Browser Toolbox settings, but
> doesn't actually show up in the Browser Toolbox (even when the setting is
> checked).

It used to work in previous patches. bug 1059308.

> 
> - On http://smartsearch.altervista.org , the switch frames button has 2
> mysterious about:blank frames that I can't switch to

I see only one and I can switch to it correctly.
Please re-test it once it reaches nightly build and do not hesitate to fill a bug.
> 
> - The switch frames command button doesn't look quite like the other command
> buttons (mainly because of the different background color)

It looks good for me on ubuntu, may be it looks not as good on other OS?

	Ryan VanderMeulen [:RyanVM]
	Comment 129 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/ceb3967c0d39
https://hg.mozilla.org/mozilla-central/rev/f74dd51cbfb9
https://hg.mozilla.org/mozilla-central/rev/0398f1390ea9
https://hg.mozilla.org/mozilla-central/rev/c3a7a750a72f
https://hg.mozilla.org/mozilla-central/rev/f14b2654c180