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)
Tracking
(firefox40+ verified, firefox41+ verified)
VERIFIED
FIXED
Firefox 41
People
(Reporter: Optimizer, Assigned: ochameau)
References
Details
(Whiteboard: [polish-backlog][difficulty=easy][bugday-20150708][bugday-20150710])
Attachments
(1 file, 1 obsolete file)
2.51 KB,
patch
|
ochameau
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•10 years ago
|
||
It's not?
Reporter | ||
Comment 2•10 years ago
|
||
devtools.command-button-frames.enabled does not exist by default.
Assignee | ||
Comment 4•10 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•10 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•10 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)
Comment 7•10 years ago
|
||
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•10 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.
Comment 9•10 years ago
|
||
(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•10 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•9 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 | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=527e29d4190d
Assignee | ||
Updated•9 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+
Comment 15•9 years ago
|
||
Let's do it!
Flags: needinfo?(jgriffiths)
Priority: -- → P1
Whiteboard: [devedition-40][difficulty=easy]
Assignee | ||
Updated•9 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•9 years ago
|
||
Attachment #8605179 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8605313 -
Flags: review+
Adding tracking flags for firefox40 and firefox41.
tracking-firefox41:
--- → +
status-firefox40:
--- → affected
status-firefox41:
--- → affected
Comment 19•9 years ago
|
||
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•9 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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Comment 23•9 years ago
|
||
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 26•9 years ago
|
||
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+
Comment 27•9 years ago
|
||
(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.
Comment 31•9 years ago
|
||
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•9 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•9 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]
Whiteboard: [devedition-40][difficulty=easy][bugday-20150708] → [devedition-40][difficulty=easy][bugday-20150708][bugday-20150710]
Updated•9 years ago
|
Whiteboard: [devedition-40][difficulty=easy][bugday-20150708][bugday-20150710] → [polish-backlog][difficulty=easy][bugday-20150708][bugday-20150710]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•