Frame selection command button should be visible by default

VERIFIED FIXED in Firefox 40

Status

()

Firefox
Developer Tools
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Optimizer, Assigned: ochameau)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 41
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40+ verified, firefox41+ verified)

Details

(Whiteboard: [polish-backlog][difficulty=easy][bugday-20150708][bugday-20150710])

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)

Comment 1

4 years ago
It's not?
(Reporter)

Comment 2

4 years ago
devtools.command-button-frames.enabled does not exist by default.

Comment 3

4 years ago
So we should turn it on.
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 4

4 years ago
If you guys thinks that's stable enough...
But I think we would like to fix storage inspector first, and look at blocking bugs of bug 977043 with all errors being reported so far. There is some issues with the highlighter and some broken about:blank document listed for nothing.
Flags: needinfo?(poirot.alex)

Comment 5

4 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> If you guys thinks that's stable enough...
> But I think we would like to fix storage inspector first

Is there a bug for that?

> and look at
> blocking bugs of bug 977043 with all errors being reported so far. There is
> some issues with the highlighter and some broken about:blank document listed
> for nothing.

Should we move these blocking/depending bugs to this bug?
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 6

4 years ago
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #5)
> (In reply to Alexandre Poirot [:ochameau] from comment #4)
> > If you guys thinks that's stable enough...
> > But I think we would like to fix storage inspector first
> 
> Is there a bug for that?

I think it is bug 1060925.

> 
> > and look at
> > blocking bugs of bug 977043 with all errors being reported so far. There is
> > some issues with the highlighter and some broken about:blank document listed
> > for nothing.
> 
> Should we move these blocking/depending bugs to this bug?

Sure, but only the ones we consider enabling it by default.
Depends on: 1060925
Flags: needinfo?(poirot.alex)
I was watching Remy Sharp trying to debug a network issue in JS Bin. He couldn't understand why, when he types in one iframe he doesn't see network requests from another iframe. It is because we only show network requests for the currently focused frame. Making this button visible by default would solve this issue.
(Reporter)

Comment 8

4 years ago
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #7)
> I was watching Remy Sharp trying to debug a network issue in JS Bin. He
> couldn't understand why, when he types in one iframe he doesn't see network
> requests from another iframe. It is because we only show network requests
> for the currently focused frame. Making this button visible by default would
> solve this issue.

If the button is not visible, that means that he did not switch to any iframe and thus is in the parent window scope. Right ?

In that case, I think its not this feature that is preventing showing of requests from other iframes.
(In reply to Girish Sharma [:Optimizer] from comment #8)
> If the button is not visible, that means that he did not switch to any
> iframe and thus is in the parent window scope. Right ?
> 

Wrong... he had a JS Bin open and was typing in the left-hand iframe expecting to see network results from the right-hand iframe (actually, from all iframes). Because he saw no network traffic he assumed there was a bug.

I mentioned this because it made a seasoned dev assume we had a bug when we don't. Focusing on a single iframe is the best thing to do but maybe we should have a visual indicator to show that there are results from other iframes... maybe turn the iframe selector to green like we do in the debugger? Of course, that should be another bug.
(Reporter)

Comment 10

4 years ago
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #9)
> (In reply to Girish Sharma [:Optimizer] from comment #8)
> > If the button is not visible, that means that he did not switch to any
> > iframe and thus is in the parent window scope. Right ?
> > 
> 
> Wrong... he had a JS Bin open and was typing in the left-hand iframe
> expecting to see network results from the right-hand iframe (actually, from
> all iframes). Because he saw no network traffic he assumed there was a bug.
> 
> I mentioned this because it made a seasoned dev assume we had a bug when we
> don't. Focusing on a single iframe is the best thing to do but maybe we
> should have a visual indicator to show that there are results from other
> iframes... maybe turn the iframe selector to green like we do in the
> debugger? Of course, that should be another bug.

My point was that since the frame selection button is not visible in the toolbox, the toolbox is ideally still targeting the parent window, which includes any and all iframes inside of the window.

Thus, even if netmonitor is not showing network requests from some iframes, its not the fault of frame selection button.
Alex, I think we should just enable this already!  It has a few edge cases, but whatever...  It's quite useful!

Jeff, any feedback one way or the other?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(jgriffiths)
(Assignee)

Comment 12

3 years ago
Oh, I meants to put that into ff40 radar but got pulled by webconsoles tweaks.
Yes I think we should do it.
Ideally bug 1049888 is going to move forward soon and may be address the main issue: storage actor not supporting iframes selection. This is the kind of panel where you want to see it working to identify what cookies set a particular (ad) iframe.
Flags: needinfo?(poirot.alex)
(Assignee)

Updated

3 years ago
Attachment #8605179 - Flags: review?(jryans)
Comment on attachment 8605179 [details] [diff] [review]
patch

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

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

Seems like you might be able to remove the BT specific one[1] now?

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox-process-window.js#57
Attachment #8605179 - Flags: review?(jryans) → review+
Let's do it!
Flags: needinfo?(jgriffiths)
Priority: -- → P1
Whiteboard: [devedition-40][difficulty=easy]
(Assignee)

Updated

3 years ago
Assignee: nobody → poirot.alex
[Tracking Requested - why for this release]:

Tracking so we don't forget to uplift to 40.
tracking-firefox40: --- → ?
(Assignee)

Comment 17

3 years ago
Created attachment 8605313 [details] [diff] [review]
patch
Attachment #8605179 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8605313 - Flags: review+
Adding tracking flags for firefox40 and firefox41.
tracking-firefox40: ? → +
tracking-firefox41: --- → +

Updated

3 years ago
status-firefox40: --- → affected
status-firefox41: --- → affected
You mention in discussion that bug 1060925 and bug 1049888 may affect this issue. Do they still? Would they affect 40 as well?
(Assignee)

Comment 20

3 years ago
(In reply to Liz Henry (:lizzard) from comment #19)
> You mention in discussion that bug 1060925 and bug 1049888 may affect this
> issue. Do they still? Would they affect 40 as well?

No, we finally decided it's not critical. We can enable it even without fixing these two bugs. It is more nice to have.
https://hg.mozilla.org/mozilla-central/rev/727955256a8e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
If I understand correctly, you don't want to have it in 40. So, wontfix and let it ride the train with 41.
status-firefox40: affected → wontfix
(In reply to Sylvestre Ledru [:sylvestre] from comment #23)
> If I understand correctly, you don't want to have it in 40. So, wontfix and
> let it ride the train with 41.

I think Alex meant to say the remaining bugs (bug 1060925 and bug 1049888) are not that big of a deal, so it should be safe to enable generally.

I had been hoping this would reach 40 with the rest of the Dev. Ed 40 effort, but might as well ask for uplift now I suppose.  Doesn't hurt to ask.  :)
status-firefox40: wontfix → affected
Comment on attachment 8605313 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Frame switching in DevTools
[User impact if declined]: Feature would be off by default if declined.  It's been around for a while, so should be safe to expose it now.
[Describe test coverage new/current, TreeHerder]:  On m-c, tests for the feature.
[Risks and why]: It enables a new menu in DevTools, should be safe.
[String/UUID change made/needed]: None
Attachment #8605313 - Flags: approval-mozilla-aurora?
Comment on attachment 8605313 [details] [diff] [review]
patch

OK. Is it worth rewriting an item in the release notes for this?
Attachment #8605313 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> Comment on attachment 8605313 [details] [diff] [review]
> patch
> 
> OK. Is it worth rewriting an item in the release notes for this?

I would rather this wait until 41 ( only a couple of weeks! ) so the change gets caught by our typical promotion process.
(In reply to Jeff Griffiths (:canuckistani) from comment #27)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> > Comment on attachment 8605313 [details] [diff] [review]
> > patch
> > 
> > OK. Is it worth rewriting an item in the release notes for this?
> 
> I would rather this wait until 41 ( only a couple of weeks! ) so the change
> gets caught by our typical promotion process.

Okay, fair enough!
status-firefox40: affected → wontfix
Well, looks like it is now in 40 anyway. :) Not a big deal, we can still promote it with 41.
status-firefox40: wontfix → fixed
Successfully reproduce the bug in 34.0a1 (2014-08-28) on Linux x64

Verified as fixed with Latest Aurora 41.0a2 (2015-07-09) (Build ID: 20150709004007) and
Latest Beta 40.0b2 (Build ID: 20150706172413)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][bugday-20150708]

Comment 32

3 years ago
I have successfully reproduced the bug in Nightly 34.0a1 (2014-08-28) with the instruction from comment 0 and windows 7 (32 bit) 
  
Verified as fixed with beta 40.0 

build ID:(20150709163524) 
Mozilla/5.0 (Windows NT 6.1; rv:40.0) Gecko/20100101 Firefox/40.0

Comment 33

3 years ago
Successfully reproduce the bug on Firefox 39.0 (20150630154324).

The fix works for me on Firefox 40.0; Build ID 	20150709163524; User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0

The fix works for me on Firefox  41.0a2; Build ID 20150711004006; User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0

Based on Comment 31 and Comment 32 and my verification I am marking this bug as verified.
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20150710]
status-firefox40: fixed → verified
status-firefox41: fixed → verified
Whiteboard: [devedition-40][difficulty=easy][bugday-20150708] → [devedition-40][difficulty=easy][bugday-20150708][bugday-20150710]
Whiteboard: [devedition-40][difficulty=easy][bugday-20150708][bugday-20150710] → [polish-backlog][difficulty=easy][bugday-20150708][bugday-20150710]
You need to log in before you can comment on or make changes to this bug.