Closed Bug 1059882 Opened 10 years ago Closed 9 years ago

Frame selection command button should be visible by default

Categories

(DevTools :: General, defect, P1)

x86_64
Windows 7
defect

Tracking

(firefox40+ verified, firefox41+ verified)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox40 + verified
firefox41 + verified

People

(Reporter: Optimizer, Assigned: ochameau)

References

Details

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

Attachments

(1 file, 1 obsolete file)

      No description provided.
It's not?
devtools.command-button-frames.enabled does not exist by default.
So we should turn it on.
Flags: needinfo?(poirot.alex)
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)
(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)
(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.
(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.
(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)
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)
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: nobody → poirot.alex
[Tracking Requested - why for this release]:

Tracking so we don't forget to uplift to 40.
Attached patch patchSplinter Review
Attachment #8605179 - Attachment is obsolete: true
Attachment #8605313 - Flags: review+
Adding tracking flags for firefox40 and firefox41.
You mention in discussion that bug 1060925 and bug 1049888 may affect this issue. Do they still? Would they affect 40 as well?
(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
Closed: 9 years ago
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.
(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.  :)
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!
Well, looks like it is now in 40 anyway. :) Not a big deal, we can still promote it with 41.
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]
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
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]
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.