After bug 1240913, we can only open RDM for remote pages. This fine for viewing web content with e10s on. However, it means it does not work for pages that load in the parent like about:addons, etc. Currently we abort and print an error to browser console, but we could improve this by using the browser's notification bar to show the error.

Helen, here's a screenshot of this message we would show when opening RDM on some non-remote tab like about:newtab or about:addons. What do you think of the text? It's a bit technical, but would only be shown to DevTools users trying to open the RDM...

Comment on attachment 8780722 [details] RDM remote only message The screenshot isn't using RDM, but I assume the intended use is that if I had opened new tab in RDM, it would give me this error? Does it appear across the top of the entire browser window, or in the phone viewport?

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #3) > Comment on attachment 8780722 [details] > RDM remote only message > > The screenshot isn't using RDM, but I assume the intended use is that if I > had opened new tab in RDM, it would give me this error? Does it appear > across the top of the entire browser window, or in the phone viewport? If you try to open RDM for a non-remote browser tab, such as something from the browser UI like about:newtab (the new tab page) or about:addons (the add-ons manager), we aren't able to open RDM for such a tab. At the moment if you try to do so, we log an error to the browser console, but there's no indication in the UI of what failed, so it would be fairly confusing, I would imagine. With the current patch, as shown in the screenshot, you would get an error notification above the browser tab content if this happens. The error notification is specific to the tab (if you switch to a different tab it is hidden until you return to the problem tab). The notification can be dismissed by clicking the X on the right hand side. It's not part of the RDM UI since it can't be shown. This notification bar is the same UI element used to display various other browser warnings specific to a tab, such as "popup blocked". You can see the popup warning version by visiting a page such as "http://www.popuptest.com/popuptest5.html".

Ahhh, okay, I understand now. It would appear when you try to open the RDM, I get it now. I'm happy with the notification and its text. My one question is, should we consider having the notification along the bottom of the page content is a user tries to open RDM with the RDM command-button, as that position would probably be more in their line of view?

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #5) > Ahhh, okay, I understand now. It would appear when you try to open the RDM, > I get it now. > > I'm happy with the notification and its text. My one question is, should we > consider having the notification along the bottom of the page content is a > user tries to open RDM with the RDM command-button, as that position would > probably be more in their line of view? I am not sure there's a clearly correct location... If you use the keyboard shortcut (Cmd-Opt-M) or menu item (Tools -> Web Dev -> RDM) to open RDM, the message could probably appear anywhere as there's not any obviously "related" part of the UI in those cases. For the command button, I see what you mean about line of view... we do have a similar notification bar inside the toolbox that we could try for the command button case. The browser UI does already have a very similar notification box that _does_ appear at the bottom of the window, but it's meant for global notifications that impact all tabs in the window, like "this add-on is slow" or "submit this crash report", so I don't think we'd want that specific UI element. I'll try assembling an new version that uses the toolbox notification bar for the command button case.

Okay, here's how the new version looks when using the toolbox button. For that case, the message is displayed as part of the toolbox, which places the message near the user's action. For other cases (shortcut, menu item), we still use the path from the previous screenshot, since there is potentially no toolbox open. Note that message appearance is a bit different between the two positions... this is because they are implemented entirely differently. The browser UI notification (top of page) is XUL component that's been around for a while. The toolbox notification is a new thing for devtools.html, so it seems to differe slightly in appearance.

Comment on attachment 8780720 [details] Bug 1274032 - Show notification when opening RDM on non-remote tabs. https://reviewboard.mozilla.org/r/71366/#review69886 Looks good to me, but there are a few things that I would like to see fixed (or acknowledged at least). By the way, I noticed that this doesn't work if the user enters a non-remote URL while using the RDM. The RDM just closes, the non-remote content loads and the tab becomes unusable (you can't set a new URL and lot of things are broken). I'll file a bug if there is not one already. ::: devtools/client/responsive.html/manager.js:40 (Diff revision 2) > * @param window > * The main browser chrome window. > * @param tab > * The browser tab. > + * @param options > + * Other options associated with toggling. Currently includes: Nit: there are 2 spaces before "Currently includes" instead of 1. ::: devtools/client/responsive.html/manager.js:62 (Diff revision 2) > * @param window > * The main browser chrome window. > * @param tab > * The browser tab. > + * @param options > + * Other options associated with opening. Currently includes: Nit: same problem. ::: devtools/client/responsive.html/manager.js:94 (Diff revision 2) > * @param window > * The main browser chrome window. > * @param tab > * The browser tab. > - * @param object > - * Close options, which currently includes a `reason` string. > + * @param options > + * Other options associated with closing. Currently includes: Nit: same problem. ::: devtools/client/responsive.html/manager.js:172 (Diff revision 2) > case "resize to": > - completed = this.openIfNeeded(window, tab); > + completed = this.openIfNeeded(window, tab, { command: true }); > this.activeTabs.get(tab).setViewportSize(args.width, args.height); > break; > case "resize on": > - completed = this.openIfNeeded(window, tab); > + completed = this.openIfNeeded(window, tab, { command: true }); Maybe you could factorize a bit here? ```javascript case "resize to": this.activeTabs.get(tab).setViewportSize(args.width, args.height); case "resize on": completed = this.openIfNeeded(window, tab, { command: true }); break; ``` This may be less readable though, so I'll let you choose. ::: devtools/client/responsive.html/manager.js:225 (Diff revision 2) > + if (toolbox) { > + nbox = toolbox.notificationBox; > + } > + } > + > + nbox.appendNotification( If the user doesn't detect the message instantly and hits the RDM shortcut a few times, this stacks notifications and it's annoying to close them all. This is not the case for toolbox notifications though. Can you check here if there's already a notification displayed and bail out early if needed, or remove it and append a new one to draw the user's attention with a new animation?

Comment on attachment 8780720 [details] Bug 1274032 - Show notification when opening RDM on non-remote tabs. https://reviewboard.mozilla.org/r/71368/#review69892

Comment on attachment 8781301 [details] RDM remote only message (toolbox button) Looks good to me!

Since :bigben's internship has ended, I'll send the next review to :gl.

Comment on attachment 8780720 [details] Bug 1274032 - Show notification when opening RDM on non-remote tabs. Let's try :ntim too. Whoever replies first... :)

I couldn't test the patch because it needs rebasing, that aside, the code changes look pretty straightforward.

Okay, should be rebased now.

Comment on attachment 8784058 [details] Bug 1274032 - Show notification when opening RDM on non-remote tabs. https://reviewboard.mozilla.org/r/73630/#review71492 Tested locally, works fine!

Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1732e27db9eb Show notification when opening RDM on non-remote tabs. r=ntim

I managed to test this fix on Firefox 51.0a1 (2016-09-01), across platforms[1] and encountered a few potential issues: - both notification error bars are displayed if trying to open RDM from both Menu bar and Toogle tools. In my opinion, if a notification error is displayed, the second one shouldn't be triggered. - hovering over the close button 'x' has not the same result on both error bars; - on Mac and Ubuntu OS's, the the 'x' icon displayed in front of the error message is not the same on both error notification bars; - the close buttons 'x' are not the same on both error notification bars; - on Mac OS, the text from notification error bars doesn't have the same format. Here is a screenshot with the potential issues described above - http://imgur.com/Lfy0Iwo. [1]Windows 10x64, Mac OS X 10.10.5, Ubuntu 16.04x64

(In reply to Mihai Boldan, QA [:mboldan] from comment #21) > I managed to test this fix on Firefox 51.0a1 (2016-09-01), across > platforms[1] and encountered a few potential issues: > - both notification error bars are displayed if trying to open RDM from both > Menu bar and Toogle tools. In my opinion, if a notification error is > displayed, the second one shouldn't be triggered. Yes, I think we could make a change to improve that. Feel free to file a new bug to do this. > - hovering over the close button 'x' has not the same result on both error > bars; > - on Mac and Ubuntu OS's, the the 'x' icon displayed in front of the error > message is not the same on both error notification bars; > - the close buttons 'x' are not the same on both error notification bars; > - on Mac OS, the text from notification error bars doesn't have the same > format. > > Here is a screenshot with the potential issues described above - > http://imgur.com/Lfy0Iwo. These are all issues caused by the two different locations using different UI components (they are implemented very differently). I have copied these issues to bug 1300128 for the group that added the DevTools notification box component to take a look at. Thanks for testing!

Since the issues described above are covered by Bug 1300128 and Bug 1300489, I am marking this bug Verified Fixed.