Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Description • 8 years ago

We need HTML version of the Notification bar

http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.xul#114

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 1 • 8 years ago

This makes sense to implement as (shared) React component.

Comments:

* The original <notificationbox> (used in toolbox.xul) is designed as a container (or wrapper) for the entire toolbox (the toolbar + toolbox body). Instead of implementing the React component as a wrapper it should be part of the layout: notification box, toolbar, body (so the notification box at the top).

* There are some tests that need to be fixed since using the notificationbox as a root element when looking for the toolbox (not many though). We should keep the root element and change its ID from toolbox-notificationbox to e.g. toolbox-box. Tests can use it instead.

* Notification is appended through toolbox.getNotificationBox().appendNotifiction()
Here is API description: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/notificationbox#m-appendNotification

* The box has several priority levels (low, hight, etc.) that use different styling to get user attention (an icons, background color, etc.).

* The box can display rich content and an image, but none of that is used in the code base so, we don't need that.

* The box always display a close button so, the user can get rid of it and set of custom buttons. Custom buttons are used by Responsive design and WebIDE. It should be fairly simple to support this. The custom buttons support access keys.

* The notification box supports more than one notification, but just one is displayed at a time AFAICAT.


Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 2 • 8 years ago

NotificationBox component

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 3 • 8 years ago

Mochitests for NotificationBar component

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 4 • 8 years ago

Browser test for NotificationBar

Honza

	Brian Grinstead [:bgrins]
	Comment 5 • 8 years ago

Comment on attachment 8748114 [details] [diff] [review]
bug1264671.patch

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

::: devtools/client/framework/toolbox.xul
@@ +111,5 @@
>        <menuitem id="cMenu_selectAll"/>
>      </menupopup>
>    </popupset>
>  
> +  <notificationbox id="toolbox-container" flex="1">

Shouldn't this xul notificationbox be turned into a box or an html:div?

@@ +112,5 @@
>      </menupopup>
>    </popupset>
>  
> +  <notificationbox id="toolbox-container" flex="1">
> +    <div xmlns="http://www.w3.org/1999/xhtml" id="toolbox-notificationbox"/>

And isn't this div unclosed?

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 6 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Shouldn't this xul notificationbox be turned into a box or an html:div?
Ah, replaced by <vbox>

> And isn't this div unclosed?
It's closed.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 7 • 8 years ago

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26a178bf538a

Honza

	Brian Grinstead [:bgrins]
	Comment 8 • 8 years ago

Screenshot of the close icon, looks like the background position is a bit off (it also shifts around when hovering)

	Brian Grinstead [:bgrins]
	Comment 9 • 8 years ago

Comment on attachment 8748197 [details] [diff] [review]
bug1264671.patch

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

Was surprised that there's no changes to any calling code here, I was assuming the API would have to change a bit but that works for me

::: devtools/client/framework/toolbox.js
@@ +29,5 @@
>  Cu.import("resource://devtools/client/scratchpad/scratchpad-manager.jsm");
>  Cu.import("resource://devtools/client/shared/DOMHelpers.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  
> +const BrowserLoaderModule = {};

Please shorten this into one command

const { BrowserLoader } = Components.utils.import("resource://devtools/client/shared/browser-loader.js", {});

@@ +369,5 @@
>          // Update the URL so that onceDOMReady watch for the right url.
>          this._URL = location;
>        }
> +
> +      let require = BrowserLoaderModule.BrowserLoader({

I'm requesting feedback from James about this initialization process for the browser loader and the way the react components are being initialized / rendered into the DOM.  James, any opinions about how this happens?  I'd like to make sure we are going to do something relatively consistent here and with things like the splitter widget which also needs to live alongside 'normal' DOM content.

@@ +374,5 @@
> +        window: this.doc.defaultView,
> +        useOnlyShared: true
> +      }).require;
> +
> +      this.React = require("devtools/client/shared/vendor/react");

If we are going to be extending this with more components, I'm thinking this chunk of code could be it's own initBrowserLoader function, and that the things being attached to the Toolbox instance directly could  be on a namespaced object (something like this.browserLoader.React, this.browserLoader.PriorityLevels, etc)

@@ +813,5 @@
> +   */
> +  _buildNotificationBox: function() {
> +    let box = this.doc.getElementById("toolbox-notificationbox");
> +
> +    // Render NotificationBox and assigne priority levels to it.

spelling: assign

@@ +2029,4 @@
>     *
> +   * @return The container box element.
> +   */
> +  getToolboxContainer: function() {

Is this used anywhere?

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 10 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Created attachment 8748798 [details]
> close-icon-off-center.png
> 
> Screenshot of the close icon, looks like the background position is a bit
> off (it also shifts around when hovering)

How can I reproduce that? I don't see it on win neither osx.

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 11 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Comment on attachment 8748197 [details] [diff] [review]
> bug1264671.patch
> 
> Review of attachment 8748197 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Was surprised that there's no changes to any calling code here, I was
> assuming the API would have to change a bit but that works for me
Keep the same API was my intention (less changes, less risk)

> 
> ::: devtools/client/framework/toolbox.js
> @@ +29,5 @@
> >  Cu.import("resource://devtools/client/scratchpad/scratchpad-manager.jsm");
> >  Cu.import("resource://devtools/client/shared/DOMHelpers.jsm");
> >  Cu.import("resource://gre/modules/Task.jsm");
> >  
> > +const BrowserLoaderModule = {};
> 
> Please shorten this into one command
> 
> const { BrowserLoader } =
> Components.utils.import("resource://devtools/client/shared/browser-loader.
> js", {});
Done

> 
> @@ +369,5 @@
> >          // Update the URL so that onceDOMReady watch for the right url.
> >          this._URL = location;
> >        }
> > +
> > +      let require = BrowserLoaderModule.BrowserLoader({
> 
> I'm requesting feedback from James about this initialization process for the
> browser loader and the way the react components are being initialized /
> rendered into the DOM.  James, any opinions about how this happens?  I'd
> like to make sure we are going to do something relatively consistent here
> and with things like the splitter widget which also needs to live alongside
> 'normal' DOM content.

Btw. I got inspired by WebConsoleFrame.

> 
> @@ +374,5 @@
> > +        window: this.doc.defaultView,
> > +        useOnlyShared: true
> > +      }).require;
> > +
> > +      this.React = require("devtools/client/shared/vendor/react");
> 
> If we are going to be extending this with more components, I'm thinking this
> chunk of code could be it's own initBrowserLoader function, and that the
> things being attached to the Toolbox instance directly could  be on a
> namespaced object (something like this.browserLoader.React,
> this.browserLoader.PriorityLevels, etc)
Yes, totally agree. We should simplify the initialization code. Let's
see how the next use case looks and share the stuff.

> 
> @@ +813,5 @@
> > +   */
> > +  _buildNotificationBox: function() {
> > +    let box = this.doc.getElementById("toolbox-notificationbox");
> > +
> > +    // Render NotificationBox and assigne priority levels to it.
> 
> spelling: assign
Fixed

> 
> @@ +2029,4 @@
> >     *
> > +   * @return The container box element.
> > +   */
> > +  getToolboxContainer: function() {
> 
> Is this used anywhere?
Removed

Honza

	Brian Grinstead [:bgrins]
	Comment 12 • 8 years ago

Comment on attachment 8749219 [details] [diff] [review]
bug1264671.patch

Resetting feedback flag

	Brian Grinstead [:bgrins]
	Comment 13 • 8 years ago

(In reply to Jan Honza Odvarko [:Honza] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > Created attachment 8748798 [details]
> > close-icon-off-center.png
> > 
> > Screenshot of the close icon, looks like the background position is a bit
> > off (it also shifts around when hovering)
> 
> How can I reproduce that? I don't see it on win neither osx.

No special STR, I'm seeing it on Yosemite.  Likely due to the use of chrome://global/skin/icons/close.png and expected 16px width/height here vs the 20px in the patch: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/global.css#338.  If we want to use the toolkit icons then we should go all the way and use the .close-icon class, which will look right across platforms.  Else we should use something like --close-button-image / url(chrome://devtools/skin/images/close.svg) which isn't a moving target.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 14 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #13)
> (In reply to Jan Honza Odvarko [:Honza] from comment #10)
> No special STR, I'm seeing it on Yosemite.  Likely due to the use of
> chrome://global/skin/icons/close.png and expected 16px width/height here vs
> the 20px in the patch:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> global.css#338.  If we want to use the toolkit icons then we should go all
> the way and use the .close-icon class, which will look right across
> platforms.  Else we should use something like --close-button-image /
> url(chrome://devtools/skin/images/close.svg) which isn't a moving target.
Yes, good point. The new patch is using our close.svg icon. It should fix the visual issue for you.

Honza

	James Long (:jlongster)
	Comment 15 • 8 years ago

Comment on attachment 8749610 [details] [diff] [review]
bug1264671.patch

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

There are probably good reasons for this, but a few things do stand out. I would just create a top-level `require` from the BrowserLoader and require everything in at the top-level. Also, avoid keeping a reference to an element instance directly.

I have to ask though (this is probably way too late): is it really worth using React for the notification bar? I only ask because React is not already available and that seems like such a simple piece that we could switch to HTML without React. However, eventually we probably want to render the tabs with React so we'll need to load it in eventually.

It is unfortunate that everything is in iframes so all libraries must be loaded multiple times. But hopefully, with all the HTML & React work going on, eventually we can reduce the iframe usage and share more things across components.

::: devtools/client/framework/toolbox.js
@@ +381,5 @@
> +        window: this.doc.defaultView,
> +        useOnlyShared: true
> +      }).require;
> +
> +      this.React = require("devtools/client/shared/vendor/react");

Why are you putting React as props here? Anything that needs to use React should just require it itself.

@@ +821,5 @@
> +  _buildNotificationBox: function() {
> +    let box = this.doc.getElementById("toolbox-notificationbox");
> +
> +    // Render NotificationBox and assigne priority levels to it.
> +    this.notificationBox = Object.assign(

Why do you keep the element instance around? It's bad practice to deal with instances directly.

All you need to do is rerender it. If data changes, just rerender the same component into the same place and it will update.

const C = createFactory(createClass(...));

React.render(C({ x: 5 }), node);
React.render(C({ x: 6 }), node);

The second `render` will not fully render it out, it is the equivalent of a `setState` call in that it will rerender the vdom and only apply the necessary changes to the DOM.

	Brian Grinstead [:bgrins]
	Comment 16 • 8 years ago

(In reply to James Long (:jlongster) from comment #15)
> It is unfortunate that everything is in iframes so all libraries must be
> loaded multiple times. But hopefully, with all the HTML & React work going
> on, eventually we can reduce the iframe usage and share more things across
> components.

Aside: we ran into some problems when merging the inspector tools into one frame around keyboard shortcuts and OSX not focusing non-form elements.  So it turns out detecting the 'focused' element isn't reliable when it's not an input and we have an OK but not great workaround in the inspector to figure out which search box should be focused when ctrl+f is hit, but I don't think that it'll scale well across tools (I could be wrong but this is what I've seen so far): https://bugzilla.mozilla.org/show_bug.cgi?id=1260235#c6.

	Brian Grinstead [:bgrins]
	Comment 17 • 8 years ago

Comment on attachment 8748117 [details] [diff] [review]
bug1264671-browser-tests.patch

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

::: devtools/client/shared/components/test/browser/browser_notification_box_basic.js
@@ +4,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const TEST_PAGE_URL = URL_ROOT + "page_basic.html";

I'd prefer to have this inlined as a string unless/until it needs to be do more and be shared:

const TEST_URI = `data:text/html;charset=utf-8,Test page`

::: devtools/client/shared/components/test/browser/head.js
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/* eslint no-unused-vars: [2, {"vars": "local", "args": "none"}] */
> +/* import-globals-from ../../../../framework/test/shared-head.js */

Can this use the shared/test/head.js instead of creating a new head.js file?  I don't feel really strongly about it, but the less code / new files the better.

@@ +10,5 @@
> +Services.scriptloader.loadSubScript(
> +  "chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js", this);
> +
> +registerCleanupFunction(() => {
> +  info("finish() was called, cleaning up...");

Is this cleanup function necessary?  We already have cleanup functions in shared-head

@@ +20,5 @@
> + *        The url to be loaded in the new tab
> + * @return a promise that resolves to the tab object when
> + *        the url is loaded
> + */
> +function addTestTab(url) {

Shouldn't the shared-head openNewTabAndToolbox function do pretty much the same here without extra code?

	Brian Grinstead [:bgrins]
	Comment 18 • 8 years ago

Comment on attachment 8749610 [details] [diff] [review]
bug1264671.patch

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

Clearing review - see James' feedback

	Brian Grinstead [:bgrins]
	Comment 19 • 8 years ago

Comment on attachment 8748116 [details] [diff] [review]
bug1264671-mochi-tests.patch

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

::: devtools/client/shared/components/test/mochitest/test_notification_box_01.html
@@ +1,1 @@
> +

Nit: newline starting all of these tests

@@ +13,5 @@
> +<body>
> +<pre id="test">
> +<script src="head.js" type="application/javascript;version=1.8"></script>
> +<script type="application/javascript;version=1.8">
> +window.onload = Task.async(function* () {

Please add a comment about what each test is covering (since they are named with numbers there's not an indication when scanning)

@@ +25,5 @@
> +
> +    // Test rendering
> +    const renderedComponent = renderComponent(NotificationBox, {});
> +    is(renderedComponent.className, "notificationbox", "NotificationBox has expected classname");
> +    is(renderedComponent.textContent, "", "Empty NotificationBox has no text content");

I think you can merge this one and test_notification_box_02.html into a single test (this one doesn't assert much)

::: devtools/client/shared/components/test/mochitest/test_notification_box_05.html
@@ +40,5 @@
> +    ok(notificationBox.getCurrentNotification(),
> +      "There must be current notification");
> +
> +    let notify = notificationBox.getNotificationWithValue("id1");
> +    notify.close();

AFAICT the only extra thing being covered in this test is the call to .close().  That could easily be merged into the end of the merged 01/02 test mentioned above.  So ultimately we'd end up with 3 tests (01, 02, and 05 all merged together)

	James Long (:jlongster)
	Comment 20 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #16)
> (In reply to James Long (:jlongster) from comment #15)
> > It is unfortunate that everything is in iframes so all libraries must be
> > loaded multiple times. But hopefully, with all the HTML & React work going
> > on, eventually we can reduce the iframe usage and share more things across
> > components.
> 
> Aside: we ran into some problems when merging the inspector tools into one
> frame around keyboard shortcuts and OSX not focusing non-form elements.  So
> it turns out detecting the 'focused' element isn't reliable when it's not an
> input and we have an OK but not great workaround in the inspector to figure
> out which search box should be focused when ctrl+f is hit, but I don't think
> that it'll scale well across tools (I could be wrong but this is what I've
> seen so far): https://bugzilla.mozilla.org/show_bug.cgi?id=1260235#c6.

That's interesting, I'd love to look into that more at some point. I still think there are significant disadvantages to iframes (namely, having to pull down extra single dependency again), but it would be good to know that advantage more (I don't understand it from a brief look). I know at some point React wants the ability to work across iframes, but I don't know how far off that is. Anyway, we'll see how this feels over time.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 21 • 8 years ago

(In reply to James Long (:jlongster) from comment #15)
> Comment on attachment 8749610 [details] [diff] [review]
> bug1264671.patch
> 
> Review of attachment 8749610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are probably good reasons for this, but a few things do stand out. I
> would just create a top-level `require` from the BrowserLoader and require
> everything in at the top-level.
Note that the `window` (needed by BrowserLoader) isn't available at the top level. This is very similar to how WebConsole uses `FrameView` (also React component), in fact I did it according to it to be consistent. Or am I missing something?

> Also, avoid keeping a reference to an
> element instance directly.
Yes, I am aware of this. I did it because I needed an object that replaces the old <notificationbox> element. An object that implements expected API so, we don't have to change the rest of the system (i.e. modules and tests that are using it) Less changes is good in the process of XUL->HTML transformation that consists for many small steps and this seemed to be good compromise. But, I agree we don't want direct access to components eventually. In any case, let me know if it isn't acceptable for you and you think it should be rewritten.

> 
> I have to ask though (this is probably way too late): is it really worth
> using React for the notification bar? I only ask because React is not
> already available and that seems like such a simple piece that we could
> switch to HTML without React. However, eventually we probably want to render
> the tabs with React so we'll need to load it in eventually.
Yeah, this is result of our decision made at the beginning. I still think it's worth it since React is being used/included in more and more places (toolbox not being an exception) and we'd be probably rewriting it again soon anyway (it's also nice adept for a component) so, why not do it now.


> 
> It is unfortunate that everything is in iframes so all libraries must be
> loaded multiple times. But hopefully, with all the HTML & React work going
> on, eventually we can reduce the iframe usage and share more things across
> components.
Definitely agree with this, we should try hard to get rid of the frames. I would be curious how much memory we could save.

> 
> ::: devtools/client/framework/toolbox.js
> @@ +381,5 @@
> > +        window: this.doc.defaultView,
> > +        useOnlyShared: true
> > +      }).require;
> > +
> > +      this.React = require("devtools/client/shared/vendor/react");
> 
> Why are you putting React as props here? Anything that needs to use React
> should just require it itself.
Yeah, done according to WebConsoleFrame
https://github.com/mozilla/gecko-dev/blob/fx-team/devtools/client/webconsole/webconsole.js#L243

I might use require in every method instead, but is it really better? (at least it's consistent now). But, I don't have a strong feeling, either way works for me.

> 
> @@ +821,5 @@
> > +  _buildNotificationBox: function() {
> > +    let box = this.doc.getElementById("toolbox-notificationbox");
> > +
> > +    // Render NotificationBox and assigne priority levels to it.
> > +    this.notificationBox = Object.assign(
> 
> Why do you keep the element instance around? It's bad practice to deal with
> instances directly.
Explained above. We might want to introduce a wrapper, if that would make it better for now.
(when all is React it should be simple to make it right).

> 
> All you need to do is rerender it. If data changes, just rerender the same
> component into the same place and it will update.
> 
> const C = createFactory(createClass(...));
> 
> React.render(C({ x: 5 }), node);
> React.render(C({ x: 6 }), node);
> 
> The second `render` will not fully render it out, it is the equivalent of a
> `setState` call in that it will rerender the vdom and only apply the
> necessary changes to the DOM.
Yes

Honza

	James Long (:jlongster)
	Comment 22 • 8 years ago

I figured that you knew all that and there were good reasons for it. My only worry is that we won't come back later and clean all this up when we don't need to keep around the old API anymore. And this pattern is discouraged, so there might be unknown problems that we'll hit. I suppose once more of the toolbox is written in React it will replace some of this need and we'll refactor it then.

No need to rewrite it or block on my feedback. Sounds like you thought this through and I don't have a better answer when dealing with backwards compatibility.

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 23 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #19)
> Comment on attachment 8748116 [details] [diff] [review]
> bug1264671-mochi-tests.patch
> 
> Review of attachment 8748116 [details] [diff] [review]:
> -----------------------------------------------------------------
Thanks Brian, new patch attached, all comments resolved.

(In reply to James Long (:jlongster) from comment #22)
> I figured that you knew all that and there were good reasons for it. My only
> worry is that we won't come back later and clean all this up when we don't
> need to keep around the old API anymore. And this pattern is discouraged, so
> there might be unknown problems that we'll hit. I suppose once more of the
> toolbox is written in React it will replace some of this need and we'll
> refactor it then.
Yes, all agreed.

Honza

	Brian Grinstead [:bgrins]
	Comment 24 • 8 years ago

Comment on attachment 8750307 [details] [diff] [review]
bug1264671-mochi-tests.patch

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

::: devtools/client/shared/components/test/mochitest/chrome.ini
@@ +15,5 @@
>  [test_tree_08.html]
>  [test_tree_09.html]
>  [test_tree_10.html]
>  [test_tree_11.html]
> +[test_notification_box_01.html]

Nit: alphabetical order

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 25 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #24)
> Comment on attachment 8750307 [details] [diff] [review]
> bug1264671-mochi-tests.patch
> 
> Review of attachment 8750307 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/components/test/mochitest/chrome.ini
> @@ +15,5 @@
> >  [test_tree_08.html]
> >  [test_tree_09.html]
> >  [test_tree_10.html]
> >  [test_tree_11.html]
> > +[test_notification_box_01.html]
> 
> Nit: alphabetical order
Fixed + some comments added

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 26 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Comment on attachment 8748117 [details] [diff] [review]
> bug1264671-browser-tests.patch
> 
> Review of attachment 8748117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> devtools/client/shared/components/test/browser/
> browser_notification_box_basic.js
> @@ +4,5 @@
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> > +
> > +const TEST_PAGE_URL = URL_ROOT + "page_basic.html";
> 
> I'd prefer to have this inlined as a string unless/until it needs to be do
> more and be shared:
> 
> const TEST_URI = `data:text/html;charset=utf-8,Test page`
Done

> 
> ::: devtools/client/shared/components/test/browser/head.js
> @@ +1,5 @@
> > +/* vim: set ts=2 et sw=2 tw=80: */
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +/* eslint no-unused-vars: [2, {"vars": "local", "args": "none"}] */
> > +/* import-globals-from ../../../../framework/test/shared-head.js */
> 
> Can this use the shared/test/head.js instead of creating a new head.js file?
> I don't feel really strongly about it, but the less code / new files the
> better.
Done

> 
> @@ +10,5 @@
> > +Services.scriptloader.loadSubScript(
> > +  "chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js", this);
> > +
> > +registerCleanupFunction(() => {
> > +  info("finish() was called, cleaning up...");
> 
> Is this cleanup function necessary?  We already have cleanup functions in
> shared-head
Removed

> 
> @@ +20,5 @@
> > + *        The url to be loaded in the new tab
> > + * @return a promise that resolves to the tab object when
> > + *        the url is loaded
> > + */
> > +function addTestTab(url) {
> 
> Shouldn't the shared-head openNewTabAndToolbox function do pretty much the
> same here without extra code?
Done

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 27 • 8 years ago

Try 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=179a8c945ff8

Honza

	Brian Grinstead [:bgrins]
	Comment 28 • 8 years ago

Comment on attachment 8749610 [details] [diff] [review]
bug1264671.patch

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

::: devtools/client/framework/toolbox.js
@@ +29,5 @@
>  Cu.import("resource://devtools/client/scratchpad/scratchpad-manager.jsm");
>  Cu.import("resource://devtools/client/shared/DOMHelpers.jsm");
>  Cu.import("resource://gre/modules/Task.jsm");
>  
> +const BrowserLoaderModule = {};

I had a comment earlier about this: please make this a single statement:

const { BrowserLoader } = Cu.import("resource://devtools/client/shared/browser-loader.js", {});

@@ +376,5 @@
>          // Update the URL so that onceDOMReady watch for the right url.
>          this._URL = location;
>        }
> +
> +      let require = BrowserLoaderModule.BrowserLoader({

Please rename to this.browserRequire (see comment below about moving notificationbox initialization into _buildNotificationBox)

@@ +384,5 @@
> +
> +      this.React = require("devtools/client/shared/vendor/react");
> +      this.ReactDOM = require("devtools/client/shared/vendor/react-dom");
> +
> +      let { NotificationBox, PriorityLevels } =

I think these lines should move into _buildNotificationBox, and then we could keep NotificationBox as a locally scoped variable (since it's not used anywhere outside of that function right now).

@@ +2036,4 @@
>     *
> +   * @return The container box element.
> +   */
> +  getToolboxContainer: function() {

You mentioned this was removed in comment 11, but it made it back

@@ +2110,5 @@
>          console.error("Panel " + id + ":", e);
>        }
>      }
>  
> +    this.React = this.ReactDOM = this.NotificationBox = null;

Going on the suggestions above, this.NotificationBox can be replaced with this.browserRequire

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 29 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #28)
> Comment on attachment 8749610 [details] [diff] [review]
> bug1264671.patch
> 
> Review of attachment 8749610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/framework/toolbox.js
> @@ +29,5 @@
> >  Cu.import("resource://devtools/client/scratchpad/scratchpad-manager.jsm");
> >  Cu.import("resource://devtools/client/shared/DOMHelpers.jsm");
> >  Cu.import("resource://gre/modules/Task.jsm");
> >  
> > +const BrowserLoaderModule = {};
> 
> I had a comment earlier about this: please make this a single statement:
Ugh, sorry, I mangled the patch attached in comment #14 (not including changes from comment #11).
All fixed now!

> 
> const { BrowserLoader } =
> Cu.import("resource://devtools/client/shared/browser-loader.js", {});
> 
> @@ +376,5 @@
> >          // Update the URL so that onceDOMReady watch for the right url.
> >          this._URL = location;
> >        }
> > +
> > +      let require = BrowserLoaderModule.BrowserLoader({
> 
> Please rename to this.browserRequire (see comment below about moving
> notificationbox initialization into _buildNotificationBox)
Done

> 
> @@ +384,5 @@
> > +
> > +      this.React = require("devtools/client/shared/vendor/react");
> > +      this.ReactDOM = require("devtools/client/shared/vendor/react-dom");
> > +
> > +      let { NotificationBox, PriorityLevels } =
> 
> I think these lines should move into _buildNotificationBox, and then we
> could keep NotificationBox as a locally scoped variable (since it's not used
> anywhere outside of that function right now).
Done

> 
> @@ +2036,4 @@
> >     *
> > +   * @return The container box element.
> > +   */
> > +  getToolboxContainer: function() {
> 
> You mentioned this was removed in comment 11, but it made it back
Related to the mistake I did in comment #14

> 
> @@ +2110,5 @@
> >          console.error("Panel " + id + ":", e);
> >        }
> >      }
> >  
> > +    this.React = this.ReactDOM = this.NotificationBox = null;
> 
> Going on the suggestions above, this.NotificationBox can be replaced with
> this.browserRequire
Done

New try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74089aacef39

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 30 • 8 years ago

Rebasing

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 31 • 8 years ago

Fixing try failure

New try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3535b8bf9885

Honza

	Jan Honza Odvarko [:Honza] (always need-info? me) Assignee
	Comment 32 • 8 years ago

The try looks good, let's try to land.

Patches should be applied in this order:
1) bug1264671.patch 
2) bug1264671-mochi-tests.patch 
3) bug1264671-browser-tests.patch 

Honza

	Pulsebot
	Comment 33 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/1a208df22802
https://hg.mozilla.org/integration/fx-team/rev/2ba629fd2361
https://hg.mozilla.org/integration/fx-team/rev/bec48bfd05a8

	Carsten Book [:Tomcat]
	Comment 34 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1a208df22802
https://hg.mozilla.org/mozilla-central/rev/2ba629fd2361
https://hg.mozilla.org/mozilla-central/rev/bec48bfd05a8

	Ada [:adalucinet]
	Comment 35 • 8 years ago

While verifying with latest Nightly 49.0a1, across platforms [1], I've found the following new issues:
- bug 1276661
- bug 1276650 
- bug 1276675

Since the new issues are tracked separately, marking here accordingly.

[1] Windows 10 64-bit, Mac OS X 10.10.5 and Ubuntu 16.04 64-bit