Julian Descottes Assignee
	Description • 9 years ago

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36

Steps to reproduce:

While working on devtools Bug 965199, I detected code duplication beetween rule-view.js and computed-view.js from the inspector panel, which makes it hard to maintain. 

Duplicated methods
============

Some identical methods :
- _isColorPopup
- _onContextMenu
- _onSelectAll
- _onToggleOrigSources
- _onClearSearch
- _onFilterTextboxContextMenu

Some other methods share similarities without being fully duplicated, so need to review them to know if they should be mutualized.

Naming inconsistencies
==============
Properties, such as the parent document, are referenced differently on the two views (.doc vs .styleDocument). Tests targeting both views therefore need to have code to handle this. We could either have a common interface or use the property names.

The views are called CssRuleView & CssHtmlTree. Unless I overlooked something, CssHtmlTree should be renamed to CssComputedView.

	Patrick Brosset <:pbro>
	Comment 1 • 9 years ago

All of this sounds perfect, something I've wanted to do for a while but never had the chance.
Would you by any chance be interested in working on this bug? That's be great.
There are several different things to refactor here, so I suggest one, small, patch is created for each change type.

Brian or Mike: do you think one of you could set yourself as a mentor on this bug?

	Julian Descottes Assignee
	Comment 2 • 9 years ago

@Patrick : sure, I'd like to do this one after I finish Bug 965199 !

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 3 • 9 years ago

(In reply to Julian Descottes from comment #2)
> @Patrick : sure, I'd like to do this one after I finish Bug 965199 !

Awesome!

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

The _onFilterTextboxContextMenu, _onClearSearch, and sort of _onContextMenu were made identical as part of the the filter styles bug 1120616.  We filed a follow up to break this out into a widget of it's own (Bug 1154789).

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

But if you have an idea for how to share these functions, it shouldn't necessarily have to wait for Bug 1154789 (we could make that bug be blocked on this work).

Mike / Julian: What's the plan for getting rid of the duplication?

	Julian Descottes Assignee
	Comment 6 • 9 years ago

@brian : Thanks for the info !

I don't know how I want to do this yet. Not sure if I should have a parent abstract view class, or if I should just extract the similar code to helpers/widgets.

I will leave the parts you mentioned out of the scope of this bug for now (from what I read in Bug 1154789 extracting this as a widget makes sense)

Will update this bug when I have a better idea of how to proceed.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 7 • 9 years ago

Just generally removing cruft and moving stuff out into widgets.

	Julian Descottes Assignee
	Comment 8 • 9 years ago

First patch is a straightforward renaming of CssHtmlTree to CssComputedView. Not much to say here, just wanted to have consistent naming between the classes used for rule-view and for computed-view.

	Julian Descottes Assignee
	Comment 10 • 9 years ago

The second patch is an attempt to move the context menu code from rule-view and computed-view to a dedicated widget. 

This means all the code related to :
- creating menu items
- adding/removing event listeners
- handling menuitem callbacks

The code is not finished/ready, but I could use feedback on the approach. 

Most of the menuitem actions are identical from one view to the other, so I simply extracted them to this context menu widget (e.g. onCopyColor, isColorPopup etc ...)

Remain in the views : 
- copy (different between the two views, and also accessible via a keyboard shortcut)
- add new rule (only available in rule view, and accessible from a link in the rule view)

Is this too much responsibility/logic for a contextmenu class ? What do you think ?

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 11 • 9 years ago

Comment on attachment 8613326 [details] [diff] [review]
bug1164343-part2.patch

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

(In reply to Julian Descottes from comment #10)
> The second patch is an attempt to move the context menu code from rule-view
> and computed-view to a dedicated widget. 
> 
> This means all the code related to :
> - creating menu items
> - adding/removing event listeners
> - handling menuitem callbacks
> 
> The code is not finished/ready, but I could use feedback on the approach. 
> 
> Most of the menuitem actions are identical from one view to the other, so I
> simply extracted them to this context menu widget (e.g. onCopyColor,
> isColorPopup etc ...)
> 
> Remain in the views : 
> - copy (different between the two views, and also accessible via a keyboard
> shortcut)
> - add new rule (only available in rule view, and accessible from a link in
> the rule view)
> 
> Is this too much responsibility/logic for a contextmenu class ? What do you
> think ?

It is actually looking really good. Don't worry too much about the number of responsibilities of the context menu class... if it is to do with the context menu it should be in there.

::: toolkit/locales/en-US/chrome/global/devtools/styleinspector.properties
@@ +75,1 @@
>  # LOCALIZATION NOTE (ruleView.contextmenu.copyColor): Text displayed in the rule

You need to make sure that all of the string names in the localization comments match their strings e.g. ruleView.contextmenu.copyColor in the comment should be styleinspector.contextmenu.copyColor

@@ +78,2 @@
>  
>  # LOCALIZATION NOTE (ruleView.contextmenu.copyColor.accessKey): Access key for

Same here.

	Julian Descottes Assignee
	Comment 12 • 9 years ago

Thanks for the feedback ! I'll go ahead with this approach.
Will mark Bug 1167598 as duplicate of this one. Fixing it here is basically free.

> You need to make sure that all of the string names in the localization comments match their strings
> e.g. ruleView.contextmenu.copyColor in the comment should be styleinspector.contextmenu.copyColor

Good catch, totally missed those !

	Julian Descottes Assignee
	Comment 14 • 9 years ago

Sorry about the lack of updates. For information I just rebased and updated my refactor to reflect the latest changes made on rule-view.js (mostly new actions coming from Bug 1024693).

Still need some work on the context menu code + writing tests.

	Julian Descottes Assignee
	Comment 15 • 9 years ago

This first patch is focused on extracting everything context-menu related in the ruleview/computedview to style-inspector-menu.js.

Many mochitests of the styleinspector package have been modified because they used to access properties/methods that have now been moved to the menu class.

	Julian Descottes Assignee
	Comment 16 • 9 years ago

This second is mostly a split+move of the rule-view file. 7 classes + utils are defined in the rule-view file, so I tried to extract all classes and put them in dedicated files.

The ruleview (and associated classes) have been moved to styleinspector/ruleview (same for computed view).

	Julian Descottes Assignee
	Comment 17 • 9 years ago

Small reminder : since Bug 1167598 is getting fixed by this change, I should also update the show MDN docs tests. They are only testing the ruleview for now, we should test both.

	Gabriel [:gl] (ΦωΦ)
	Comment 18 • 9 years ago

Just some comments from looking through some of the changes since I am interested in ensuring we preserve the order of the context menus in the rule view. I think it would be a lot easier to review if we split the unit test into a different patch/part.

	Julian Descottes Assignee
	Comment 19 • 9 years ago

@Gabriel : good point, the patch files are too big for review. I will clean them up as you suggested and resubmit them.

	Julian Descottes Assignee
	Comment 20 • 9 years ago

Initial commit extracting all the context menu logic to a separate class. Mochitests fixes are in another patch.

	Julian Descottes Assignee
	Comment 21 • 9 years ago

Mochitests fixes for the previous patch.

	Julian Descottes Assignee
	Comment 22 • 9 years ago

This next part was not really discussed in the bug but I extracted all the inner classes of the ruleview to separate files, to lighten rule-view.js.

Let me know what you think.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 23 • 9 years ago

Comment on attachment 8628452 [details] [diff] [review]
Bug1164343-part1-initial-commit.patch

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

r+ with a few nits... addressing them is optional.

::: browser/devtools/styleinspector/computed-view.js
@@ +710,4 @@
>     */
>    _onCopy: function(event)
>    {
> +    this.copySelection(event.target);

This should be inside the if (event) { block.

::: browser/devtools/styleinspector/rule-view.js
@@ +20,5 @@
>  const {parseSingleValue, parseDeclarations} =
>        require("devtools/styleinspector/css-parsing-utils");
>  const overlays = require("devtools/styleinspector/style-inspector-overlays");
>  const EventEmitter = require("devtools/toolkit/event-emitter");
> +const StyleInspectorMenu = require("devtools/styleinspector/style-inspector-menu");

loader.lazyRequireGetter(this, "StyleInspectorMenu", "devtools/styleinspector/style-inspector-menu");

Maybe you could also update overlays and EventEmitter (above) too. This is just a newer way of doing things but a good habit to build.

@@ +1408,5 @@
> +   * Callback for copy event
> +   * @param {Event} event copy event object.
> +   */
> +  _onCopy: function(event) {
> +    this.copySelection(event.target);

This should be inside the if (event) { block.

::: browser/devtools/styleinspector/style-inspector-menu.js
@@ +1,5 @@
> +"use strict";
> +
> +const {Cc, Ci, Cu} = require("chrome");
> +const {PREF_ORIG_SOURCES} = require("devtools/styleeditor/utils");
> +const overlays = require("devtools/styleinspector/style-inspector-overlays");

loader.lazyRequireGetter(this, "overlays", "devtools/styleinspector/style-inspector-overlays");

@@ +3,5 @@
> +const {Cc, Ci, Cu} = require("chrome");
> +const {PREF_ORIG_SOURCES} = require("devtools/styleeditor/utils");
> +const overlays = require("devtools/styleinspector/style-inspector-overlays");
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

You don't need this.

@@ +4,5 @@
> +const {PREF_ORIG_SOURCES} = require("devtools/styleeditor/utils");
> +const overlays = require("devtools/styleinspector/style-inspector-overlays");
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

loader.lazyImporter(this, "Services", "resource://gre/modules/Services.jsm");

@@ +54,5 @@
> +    popupset.appendChild(this._menupopup);
> +  },
> +
> +  show: function(event) {
> +    try {

Not sure if we really need this try catch.

@@ +492,5 @@
> +    this.view = null;
> +  }
> +};
> +
> +XPCOMUtils.defineLazyGetter(this, "clipboardHelper", function() {

Moe this to the top of the file and do it like this:
loader.lazyServiceGetter(this, "clipboardHelper", "@mozilla.org/widget/clipboardhelper;1", "nsIClipboardHelper");

@@ +497,5 @@
> +  return Cc["@mozilla.org/widget/clipboardhelper;1"].
> +    getService(Ci.nsIClipboardHelper);
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "_strings", function() {

I would move this to the top too:
loader.lazyGetter(this, "_strings", () => {
  return Services.strings
  .createBundle("chrome://global/locale/devtools/styleinspector.properties");
});

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 24 • 9 years ago

Comment on attachment 8628455 [details] [diff] [review]
Bug1164343-part3-split-ruleview.patch

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

Awesome work!

We have been trying to get rid of Cu.imports where we can so I have given instructions where I can to help with this (that covers a bulk of my comments).

There are a few console.logs which I guess you are aware of.

In one place we create hidden DOM windows just so that we can use timers so I have commented about using timer.jsm for that instead.

::: browser/devtools/styleinspector/ruleview/element-style.js
@@ +12,5 @@
> +const {Rule} = require("devtools/styleinspector/ruleview/rule");
> +const {promiseWarn} = require("devtools/styleinspector/ruleview/utils");
> +const {PSEUDO_ELEMENTS} = require("devtools/server/actors/styles");
> +
> +Cu.import("resource://gre/modules/Services.jsm");

loader.lazyImporter(this, "Services", "resource://gre/modules/Services.jsm");

::: browser/devtools/styleinspector/ruleview/rule-editor.js
@@ +16,5 @@
> +const {Rule} = require("devtools/styleinspector/ruleview/rule");
> +const {InplaceEditor, editableField, editableItem} =
> +      require("devtools/shared/inplace-editor");
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

You don't need to import XPCOMUtils.

@@ +17,5 @@
> +const {InplaceEditor, editableField, editableItem} =
> +      require("devtools/shared/inplace-editor");
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

loader.lazyImporter(this, "Services", "resource://gre/modules/Services.jsm");

@@ +465,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "_strings", function() {
> +  return Services.strings.createBundle(
> +    "chrome://global/locale/devtools/styleinspector.properties");
> +});
\ No newline at end of file

Move this to the top and use:
loader.lazyGetter(this, "_strings", () => {
  return Services.strings
    .createBundle("chrome://global/locale/devtools/styleinspector.properties");
});

::: browser/devtools/styleinspector/ruleview/rule-view.js
@@ +29,2 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");

loader.lazyImporter(this, "Services", "resource://gre/modules/Services.jsm");

@@ +29,3 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");

You don't need to import XPCOMUtils.

@@ +41,5 @@
>  // This is used to parse user input when filtering.
>  const FILTER_PROP_RE = /\s*([^:\s]*)\s*:\s*(.*?)\s*;?$/;
>  
>  const IOService = Cc["@mozilla.org/network/io-service;1"]
>                    .getService(Ci.nsIIOService);

loader.lazyServiceGetter(this, "IOService", "@mozilla.org/network/io-service;1", "nsIIOService")

@@ +101,5 @@
>   *        The PageStyleFront for communicating with the remote server.
>   * @constructor
>   */
>  function CssRuleView(aInspector, aDocument, aStore, aPageStyle) {
> +  console.log('isntanciate');

console.log... this should be removed.

@@ +172,5 @@
>  
>    this._contextmenu = new StyleInspectorMenu(this);
>    this._contextmenu.addToView();
>  
> +  console.log('rule view : before');

console.log... this should be removed.

@@ +178,5 @@
>    this.tooltips = new overlays.TooltipsOverlay(this);
>    this.tooltips.addToView();
>    this.highlighters = new overlays.HighlightersOverlay(this);
>    this.highlighters.addToView();
> +  console.log('rule view : after');

console.log... this should be removed.

@@ +1205,5 @@
>  
>  XPCOMUtils.defineLazyGetter(this, "clipboardHelper", function() {
>    return Cc["@mozilla.org/widget/clipboardhelper;1"]
>      .getService(Ci.nsIClipboardHelper);
>  });

Move this to the top and use loader.lazyServiceGetter(this, "clipboardHelper", "@mozilla.org/widget/clipboardhelper;1", "nsIClipboardHelper");

@@ +1211,4 @@
>  XPCOMUtils.defineLazyGetter(this, "_strings", function() {
>    return Services.strings.createBundle(
>      "chrome://global/locale/devtools/styleinspector.properties");
>  });

Move this to the top and use:
loader.lazyGetter(this, "_strings", () => {
  return Services.strings
    .createBundle("chrome://global/locale/devtools/styleinspector.properties");
});

@@ +1214,5 @@
>  });
>  
>  XPCOMUtils.defineLazyGetter(this, "domUtils", function() {
>    return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
>  });

Move this to the top and use:
loader.lazyServiceGetter(this, "domUtils", "@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

::: browser/devtools/styleinspector/ruleview/rule.js
@@ +14,5 @@
> +const {promiseWarn} = require("devtools/styleinspector/ruleview/utils");
> +const {parseDeclarations} = require("devtools/styleinspector/css-parsing-utils");
> +
> +const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

You don't need to import XPCOMUtils.

@@ +570,5 @@
> +    return selectorText + " {" + terminator + cssText + "}";
> +  }
> +};
> +
> +XPCOMUtils.defineLazyGetter(this, "domUtils", function() {

Move this to the top and use:
loader.lazyServiceGetter(this, "domUtils", "@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

@@ +574,5 @@
> +XPCOMUtils.defineLazyGetter(this, "domUtils", function() {
> +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "osString", function() {

Move this to the top too and use:
loader.lazyGetter(this, "osString", () => {
  return Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).OS;
});

::: browser/devtools/styleinspector/ruleview/text-property-editor.js
@@ +21,5 @@
> +  HTML_NS
> +} = require("devtools/styleinspector/ruleview/utils");
> +const {InplaceEditor, editableField} = require("devtools/shared/inplace-editor");
> +const IOService = Cc["@mozilla.org/network/io-service;1"]
> +                  .getService(Ci.nsIIOService);

loader.lazyServiceGetter(this, "IOService", "@mozilla.org/network/io-service;1", "nsIIOService")

@@ +23,5 @@
> +const {InplaceEditor, editableField} = require("devtools/shared/inplace-editor");
> +const IOService = Cc["@mozilla.org/network/io-service;1"]
> +                  .getService(Ci.nsIIOService);
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

You don't need to import XPCOMUtils.

@@ +670,5 @@
> +};
> +
> +XPCOMUtils.defineLazyGetter(this, "domUtils", function() {
> +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +});
\ No newline at end of file

\ No newline at end of file

Move this to the top and use:
loader.lazyServiceGetter(this, "domUtils", "@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

::: browser/devtools/styleinspector/ruleview/text-property.js
@@ +7,5 @@
> + "use strict";
> +
> +const {Cc, Ci, Cu} = require("chrome");
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

You don't need to import XPCOMUtils.

@@ +154,5 @@
> +};
> +
> +XPCOMUtils.defineLazyGetter(this, "domUtils", function() {
> +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +});
\ No newline at end of file

Move this to the top and use:
loader.lazyServiceGetter(this, "domUtils", "@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

::: browser/devtools/styleinspector/ruleview/utils.js
@@ +9,5 @@
> +const {Cc, Ci, Cu} = require("chrome");
> +const {parseDeclarations} = require("devtools/styleinspector/css-parsing-utils");
> +const {Promise: promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
> +
> +Cu.import("resource://gre/modules/Services.jsm");

You don't need to import Services as it is only used here to create a hidden DOM window and set Timers... we should use timer.jsm for this instead as we then avoid creating hidden DOM windows:
const {setTimeout, clearTimeout} = Cu.import("resource://gre/modules/Timer.jsm", {});

@@ +66,5 @@
> +function clearTimeout() {
> +  let win = Services.appShell.hiddenDOMWindow;
> +  return win.clearTimeout.apply(win, arguments);
> +}
> +exports.clearTimeout = clearTimeout;

-function setTimeout() {
-  let win = Services.appShell.hiddenDOMWindow;
-  return win.setTimeout.apply(win, arguments);
-}
exports.setTimeout = setTimeout;
 
-function clearTimeout() {
-  let win = Services.appShell.hiddenDOMWindow;
-  return win.clearTimeout.apply(win, arguments);
-}
exports.clearTimeout = clearTimeout;

@@ +148,5 @@
> +exports.advanceValidate = advanceValidate;
> +
> +XPCOMUtils.defineLazyGetter(this, "domUtils", function() {
> +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +});
\ No newline at end of file

Not sure how this ever worked considering that XPCOMUtils isn't defined here.

Anyhow, move this to the top and use:
loader.lazyServiceGetter(this, "domUtils", "@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

::: browser/devtools/styleinspector/style-inspector-menu.js
@@ +7,5 @@
>  "use strict";
>  
>  const {Cc, Ci, Cu} = require("chrome");
>  const {PREF_ORIG_SOURCES} = require("devtools/styleeditor/utils");
>  const overlays = require("devtools/styleinspector/style-inspector-overlays");

loader.lazyRequireGetter(this, "overlays", "devtools/styleinspector/style-inspector-overlays");

@@ +9,5 @@
>  const {Cc, Ci, Cu} = require("chrome");
>  const {PREF_ORIG_SOURCES} = require("devtools/styleeditor/utils");
>  const overlays = require("devtools/styleinspector/style-inspector-overlays");
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");

You don't need to import XPCOMUtils.

@@ +10,5 @@
>  const {PREF_ORIG_SOURCES} = require("devtools/styleeditor/utils");
>  const overlays = require("devtools/styleinspector/style-inspector-overlays");
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");

loader.lazyImporter(this, "Services", "resource://gre/modules/Services.jsm");

@@ +515,5 @@
>  
>  XPCOMUtils.defineLazyGetter(this, "clipboardHelper", function() {
>    return Cc["@mozilla.org/widget/clipboardhelper;1"].
>      getService(Ci.nsIClipboardHelper);
>  });

Move this to the top and use loader.lazyServiceGetter(this, "clipboardHelper", "@mozilla.org/widget/clipboardhelper;1", "nsIClipboardHelper");

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +271,5 @@
>      // MDN CSS help tooltip
>      this.cssDocs = new CssDocsTooltip(this.view.inspector.panelDoc);
>  
>      if (this.isRuleView) {
> +      console.log('addToView : isRuleView : TOOLTIPS');

console.logs should be removed.

@@ +296,5 @@
>      this.previewTooltip.stopTogglingOnHover(this.view.element);
>      this.previewTooltip.destroy();
>  
> +    if (this.isRuleView) {
> +      console.log('removeFromView : isRuleView : TOOLTIPS');

console.logs should be removed.

	Julian Descottes Assignee
	Comment 25 • 9 years ago

Thanks for the fast feedback ! 

I will integrate your remarks, perform a last rebase and submit a new patch.
Do you prefer a single patch with 1 commit here or should I keep my current split in 3 commits ?

	Patrick Brosset <:pbro>
	Comment 26 • 9 years ago

(In reply to Julian Descottes from comment #25)
> Thanks for the fast feedback ! 
> 
> I will integrate your remarks, perform a last rebase and submit a new patch.
> Do you prefer a single patch with 1 commit here or should I keep my current
> split in 3 commits ?
Splitting the work in several patches to help review is a very good idea. Once done, the split should be preserved along the whole review process, so the reviewer can easily interdiff each patch.
Before landing though, we should make sure commits are self-contained so that if someone does a 'hg up' on any of the commits, it still builds and tests pass fine.
For example, it's often good practice to split code changes in commit 1 and test changes in commit 2 to help review but if that means tests fail if only commit 1 is applied, then those commits should be folded before landing.

	Gabriel [:gl] (ΦωΦ)
	Comment 27 • 9 years ago

Comment on attachment 8628452 [details] [diff] [review]
Bug1164343-part1-initial-commit.patch

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

::: browser/devtools/styleinspector/computed-view.js
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// "use strict";
> +

This should be "use strict"; without the "//"

@@ +130,5 @@
>   *        the style information.
>   *
>   * @constructor
>   */
> +function CssComputedView(aInspector, aDocument, aPageStyle)

I think this would also be a good opportunity to also remove the aArgs style.

So, instead of aInspector, aDocument, aPageStyle it should be inspector, document, pageStyle.

@@ +135,2 @@
>  {
> +  this.styleDocument = aDocument;

I am not a big fan of this.styleDocument and this.styleWindow and would prefer it to be less verbose and have it be named this.doc and this.win / this.window.

I will leave that up to you on whether or not you want to include this with your changes, and see what others think.

@@ +135,4 @@
>  {
> +  this.styleDocument = aDocument;
> +  this.styleWindow = this.styleDocument.defaultView;
> +  this.inspector = aInspector;

I think you want to keep this.styleInspector and this.inspector and keep the exact same method signature when constructing CssComputedView as before. 

You will see this.styleInspector is needed in line 659.

@@ +615,5 @@
>                                   CssLogic.FILTER.UA :
>                                   CssLogic.FILTER.USER;
>    },
>  
> +  _onSourcePrefChanged: function CssComputedView__onSourcePrefChanged()

Let's carry over the comment about this function from when we bind this function:

// Update source links when pref for showing original sources changes

@@ +707,2 @@
>    /**
> +   * Callback for copy event

We should be a bit more descriptive about what the callback function actually does. I think it would be sensible to keep the original doc description about copying selected text.

@@ -1024,5 @@
>      // The element that we're inspecting, and the document that it comes from.
>      this.propertyViews = null;
>      this.styleWindow = null;
>      this.styleDocument = null;
> -    this.styleInspector = null;

Keep this.styleInspector = null.

@@ +1153,5 @@
>          function matchedSelectorViews_convert(aSelectorInfo) {
>            this._matchedSelectorViews.push(new SelectorView(this.tree, aSelectorInfo));
>          }, this);
>      }
> +    console.log('matchedSelectorViews length : ' +  this._matchedSelectorViews.length);

Remove console.log

::: browser/devtools/styleinspector/rule-view.js
@@ +1152,5 @@
>  
>    this._editorsExpandedForFilter = [];
>    this._outputParser = new OutputParser();
>  
> +  this.addRule = this.addRule.bind(this);

I think this should remain as this._onAddRule to remain consistent.

@@ +1389,5 @@
>  
>      return {type, value};
>    },
>  
> +  _getRuleEditorForNode : function (node) {

Remove the space between function (node).

You may want to install eslint so you can lint your changes, and ensure your code meets the coding standards. https://wiki.mozilla.org/DevTools/CodingStandards

::: browser/devtools/styleinspector/style-inspector-menu.js
@@ +1,1 @@
> +"use strict";

Add the following to the top file:

/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
/* vim: set ts=2 et sw=2 tw=80: */
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

@@ +141,5 @@
> +      command: this._onShowMdnDocs
> +    });
> +
> +    // Add new rule
> +    this.menuitemAddRule = this.createMenuItem({

The ordering of the add rule context menu is a bit off with add rule being added after Show Sources and Show MDN Docs.

this.menuitemAddRule should appear after this.createMenuSeparator and this.menuitemSources.

@@ +151,5 @@
> +
> +  createMenuItem : function(aAttributes) {
> +    let ownerDocument = this._menupopup.ownerDocument;
> +    let item = ownerDocument.createElementNS(XUL_NS, "menuitem");
> +    // console.log(aAttributes.label, aAttributes.accesskey);

Remove console.log

@@ +153,5 @@
> +    let ownerDocument = this._menupopup.ownerDocument;
> +    let item = ownerDocument.createElementNS(XUL_NS, "menuitem");
> +    // console.log(aAttributes.label, aAttributes.accesskey);
> +
> +    var selector = {

Use 'let' instead of 'var'

@@ +157,5 @@
> +    var selector = {
> +      test : 'test'
> +    };
> +
> +    var obj = {

Use 'let' instead of 'var'

@@ +188,5 @@
> +   * Update the context menu. This means enabling or disabling menuitems as
> +   * appropriate.
> +   */
> +  _updateMenuItems: function()
> +  {

Move '{' next to function()

	Gabriel [:gl] (ΦωΦ)
	Comment 28 • 9 years ago

Comment on attachment 8628455 [details] [diff] [review]
Bug1164343-part3-split-ruleview.patch

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

::: browser/devtools/shared/test/unit/test_advanceValidate.js
@@ +10,5 @@
>  const Cu = Components.utils;
>  const Ci = Components.interfaces;
>  let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
>  let require = devtools.require;
> +let {advanceValidate} = require("devtools/styleinspector/ruleview/utils");

this should be _advanceValidate

::: browser/devtools/styleinspector/ruleview/rule-editor.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

Trim the left whitespace

::: browser/devtools/styleinspector/ruleview/rule.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

Trim left whitespace

::: browser/devtools/styleinspector/ruleview/text-property-editor.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

Trim left whitespace

::: browser/devtools/styleinspector/ruleview/text-property.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> + "use strict";

Trim left whitespace

::: browser/devtools/styleinspector/ruleview/user-properties.js
@@ +2,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Add "use strict";

	Gabriel [:gl] (ΦωΦ)
	Comment 29 • 9 years ago

Hi Julian,

The changes are looking real good. Good job! I am noticing some errors as pointed out above which could be easily fixed with a linter. We use eslint in our codebase now so that should be really helpful in avoiding errors. I feel we might want to do a try push to catch any lingering errors since your refactoring are growing.

	Gabriel [:gl] (ΦωΦ)
	Comment 30 • 9 years ago

I also believe you might want to start landing sooner rather than later so you don't have to keep rebasing your changes. You might want to focus on tidying up part 1 and 2 and land those first.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 31 • 9 years ago

pbrosset answered your question in comment 26

	Patrick Brosset <:pbro>
	Comment 32 • 9 years ago

For info, bug 1130330 adds one more context menu item to the rule and computed views, and it looks like it will probably land before this. So prepare for some merging!

	Julian Descottes Assignee
	Comment 33 • 9 years ago

:gl Thanks for the review ! 

> I am not a big fan of this.styleDocument and this.styleWindow and would prefer it to be less verbose and have it be named this.doc and this.win / this.window.

Not a huge fan either but I think verbose names are helpful when searching, since we have to rely on text search. One added value is that if I want to change "styleDocument" to "newName", it will be easy to do a simple find+replace.

> We use eslint in our codebase now so that should be really helpful in avoiding errors.

I'll setup eslint and will cleanup my patch.

> I also believe you might want to start landing sooner rather than later so you don't have to keep rebasing your changes

Totally agree. (I'd prefer to land everything in one shot though, if possible)

	Julian Descottes Assignee
	Comment 34 • 9 years ago

This patch contains : 
- the 3 previous commits already reviewed
- rebased 10 minutes ago
- a 4th commit containing the changes based on review comments from Mike and Gabriel

	Julian Descottes Assignee
	Comment 35 • 9 years ago

This patch only contains the review changes if you prefer.
Let me know if you want me to upload a merged commit.

	Julian Descottes Assignee
	Comment 36 • 9 years ago

Also, I think it will be best to wait until Bug 1130330 is checked in so that I can merge it manually. 

Do you know other bugs currently active could be impacted by this one ?

	Gabriel [:gl] (ΦωΦ)
	Comment 37 • 9 years ago

I think the current patches are a bit difficult to review to ensure correctness. I would recommend separating the patches to very refine refactoring goal. 

A possible first part of the refactor could just be limited to refactoring the context menu only. Additional follows up to refactor the rules, element style, text property editor, etc, but each of these refactors should be its own patch.

There are a lot of changes and removals in some of these patches, which I am not entirely sure are correct, but that is in part due to the increasing complexity in each of these patches.

I would also highly recommend landing quickly to limit the amount of rebasing.

	Gabriel [:gl] (ΦωΦ)
	Comment 38 • 9 years ago

Comment on attachment 8629793 [details] [diff] [review]
Bug1164343-review.patch

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

Good work with the refactoring so far. We are getting closer to what we want in the refactors.

It is still a bit difficult to review due to the numbers of changes happening at the same time.

To make it easier, I would avoid fixing eslint errors you are seeing that don't affect the refactor. We will definitely have a eslint cleanup, but we might want to block it until this refactoring is done. I would avoid changing what the current functions are doing. We want to focus on moving the functions/classes to the right place, and doing any follows up if needed.

::: browser/devtools/styleinspector/computedview/computed-view.js
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/*globals _Iterator, _strings, overlays, promise, StyleInspectorMenu,
> +          gDevTools, Services, template, clipboardHelper*/

Needs a space between clipboardHelper and */

::: browser/devtools/styleinspector/ruleview/element-style.js
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/*globals promise, Services*/

Needs spacing

::: browser/devtools/styleinspector/ruleview/rule-editor.js
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/*globals _strings, Services */

Needs a space

::: browser/devtools/styleinspector/ruleview/rule-view.js
@@ +13,5 @@
> +  overlays,
> +  EventEmitter
> +  StyleInspectorMenu,
> +  clipboardHelper
> +*/

I think we should be consistent and have globals inline instead of having each item on its own line.

@@ +50,4 @@
>  const PREF_DEFAULT_COLOR_UNIT = "devtools.defaultColorUnit";
>  const PREF_ENABLE_MDN_DOCS_TOOLTIP = "devtools.inspector.mdnDocsTooltip.enabled";
> +const PREF_UA_STYLES = "devtools.inspector.showUserAgentStyles";
> +const RULEVIEW_PROPERTYNAME_CLASS = "ruleview-propertyname";

I think we should remove this const completely. We are not consistent with how we check if an element contains a particularly class using constants, so, we might as well just replace all instances of RULEVIEW_PROPERTYNAME_CLASS with the actual value and remove the constant.

@@ +389,5 @@
>  
>        if (target && target.nodeName == "input") {
>          // window.getSelection cannot be used if the target is an input
> +        let start = Math.min(target.selectionEnd, target.selectionStart);
> +        let end = Math.max(target.selectionEnd, target.selectionStart);

Math.min and Math.max seems a bit unnecessary since the following should be true selectionStart < selectionEnd for the target selection.

@@ +677,5 @@
>          promises.push(rule._applyingModifications);
>        }
>      }
>  
> +    promise.all(promises).then(() => {

The return was removed here.

::: browser/devtools/styleinspector/ruleview/text-property-editor.js
@@ +707,5 @@
> +
> +  /**
> +   * Append a text node to an element.
> +   */
> +  _appendText: function(aParent, aText) {

I don't think _appendText needs to be classed as a private method.

::: browser/devtools/styleinspector/utils.js
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* globals promise*/

Add a space between promise and */

@@ +14,2 @@
>  
> +const HTML_NS = "http://www.w3.org/1999/xhtml";

Group the const together. Move const HTML_NS after const {Cc, Ci, Cu}

@@ -103,5 @@
> - */
> -function appendText(aParent, aText) {
> -  aParent.appendChild(aParent.ownerDocument.createTextNode(aText));
> -}
> -exports.appendText = appendText;

I don't think we want to remove appendText from utils.js since this is definitely a util function that could be reused in any of the files in the styleinspector. I would also consider keeping the rest of the functions removed in utils.js

	Julian Descottes Assignee
	Comment 39 • 9 years ago

> A possible first part of the refactor could just be limited to refactoring the context menu only.

The feedback+ I got from Mike on the "split" patch led me to think I should land the refactor of the context menu and the split at the same time.

@Mike : can we just get a quick confirmation I should do as Gabriel mentioned ? 

> There are a lot of changes and removals in some of these patches, 
> which I am not entirely sure are correct, but that is in part due to the increasing 
> complexity in each of these patches.

I know it can't catch everything, but I'm regularly running styleInspector and inspector test suites.
I didn't do it after my last rebase today, but I did it yesterday (after a rebase on Friday). Now if you have specific examples you'd like to discuss please let me know. 

> Additional follows up to refactor the rules, element style, text property editor, etc, 
> but each of these refactors should be its own patch.

The classes were already split in ruleview.js, I simply extracted them in different files. I also fixed errors and warnings coming from eslint. But there is no architecture change here.

	Gabriel [:gl] (ΦωΦ)
	Comment 40 • 9 years ago

(In reply to Julian Descottes from comment #39)
> > A possible first part of the refactor could just be limited to refactoring the context menu only.
> 
> The feedback+ I got from Mike on the "split" patch led me to think I should
> land the refactor of the context menu and the split at the same time.
> 
> @Mike : can we just get a quick confirmation I should do as Gabriel
> mentioned ? 
> 

A feedback+ might be a bit too early to merge back all the parts since we need to still review any additional follow up changes given and approve it with a r+ before the patch can land. Typically you would make the changes in the same patch and we would easily diff the changes between the versions.

> > There are a lot of changes and removals in some of these patches, 
> > which I am not entirely sure are correct, but that is in part due to the increasing 
> > complexity in each of these patches.
> 
> I know it can't catch everything, but I'm regularly running styleInspector
> and inspector test suites.
> I didn't do it after my last rebase today, but I did it yesterday (after a
> rebase on Friday). Now if you have specific examples you'd like to discuss
> please let me know. 
>

My main concerns are with the returns and changes to the logic of the functions. Some examples are outlined in my last review of the code: (1) removal of returns, (2) Math.min and Math.max logic changes. 

> > Additional follows up to refactor the rules, element style, text property editor, etc, 
> > but each of these refactors should be its own patch.
> 
> The classes were already split in ruleview.js, I simply extracted them in
> different files. I also fixed errors and warnings coming from eslint. But
> there is no architecture change here.

My goal was to generate small modular reviewable patches. Each of these classes (rule, element style, etc) that are refactored are already quite sizeable to be in its own patch. It would be easier to review a large number of small patches so we can easily keep track of the changes and we can also easily land them earlier than waiting for rebases. Another example of its benefits would be if you were refactoring element style and there was a follow up comment for changes in text property editor, you would only need to change the patch containing text property editor and your r+ element style patch would be able to land without being blocked on these follow up or possibility rebases needed in the future.

There is no doubt in my mind that landing smaller patches earlier is the way to go since the styleinspector is very active in terms of changes being landed, and a lot of time would otherwise be spent rebasing your patch. I would avoid one large single patch altogether and keep all the patches small and modular with redefined goals of what you are refactoring. It is not uncommon to have 13 patches in a patch. When a patch is 100-250 lines of changes, that is a lot easier to review (numbers are made up).

	Julian Descottes Assignee
	Comment 41 • 9 years ago

Sure, I'll submit a new patch.

	Julian Descottes Assignee
	Comment 42 • 9 years ago

This attachment only contains the extraction of the context menu logic to a separate file, as well as the integration of the review comments from Mike and Gabriel concerning this part.

Tried to keep the changes to a minimum.

	Julian Descottes Assignee
	Comment 43 • 9 years ago

This attachment only contains mochitest fixes.

	Gabriel [:gl] (ΦωΦ)
	Comment 44 • 9 years ago

Comment on attachment 8630115 [details] [diff] [review]
Bug1164343-part1.v1.patch

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

Hi Julian,

Sorry for the delay in the review. We are getting really close, and all of the changes look good! There are some minor feedback that needs to be addressed, afterwards, please r? mike for the final review.

On top of that, I think we should consider getting you try push privileges. This would allow you to push to a try server which will automatically run the testsuites. You should still run the testsuite locally for the relevant component you are working on, but the try push will allow for a full test coverage. The first step would be to request committer access level 1.

See https://wiki.mozilla.org/ReleaseEngineering/TryServer for information about how to push to the try server.
See https://www.mozilla.org/en-US/about/governance/policies/commit/ for information about how to request committer access.

::: browser/devtools/styleinspector/computed-view.js
@@ +192,2 @@
>    this.createStyleViews();
> +  this._contextmenu = new StyleInspectorMenu(this);

Add a space between this.createStyleViews() and this._contextmenu

@@ +765,3 @@
>        this._contextmenu = null;
>      }
> +    this.styleDocument.popupNode = null;

This appears in both rule-view.js and computed-view.js but I think it should be in the destroy method in style-inspector-menu.js

@@ +809,5 @@
>      this.propertyViews = null;
>      this.styleWindow = null;
>      this.styleDocument = null;
> +
> +    this.destroyed = true;

I know this was added in place of this.styleInspector since it got removed in the initialization, but I think it would still be a good idea to keep this.styleInspector in case we have further interaction between the inspector and computed view in the future.

::: browser/devtools/styleinspector/rule-view.js
@@ +1157,2 @@
>    this._onCopy = this._onCopy.bind(this);
> +

Remove this space.

@@ +1163,3 @@
>    this._onTogglePseudoClassPanel = this._onTogglePseudoClassPanel.bind(this);
>    this._onTogglePseudoClass = this._onTogglePseudoClass.bind(this);
> +  let doc = this.styleDocument;

Add a space to separate the last function that gets bind, and doc

@@ +1214,2 @@
>    this._showEmpty();
> +  this._contextmenu = new StyleInspectorMenu(this);

Add a space after this._showEmpty().

@@ +1389,5 @@
>        return null;
>      }
>      return {type, value};
>    },
> +  _getRuleEditorForNode: function(node) {

Add a space to separate out the function defintions

@@ +1415,5 @@
>    },
>    /**
> +   * Copy the current selection. The current target is necessary
> +   * if the selection is inside an input or a textarea
> +   * @param  {DOMNode} target DOMNode target of the copy action

There are 2 spaces between @param and {DOMNode}. Just need a single space here.

@@ -1672,5 @@
> -      let text;
> -
> -      if (event.target.nodeName === "menuitem") {
> -        target = this.doc.popupNode;
> -      }

I am unsure about this removal, but maybe Mike might have a better idea why this was needed originally.

@@ +1436,5 @@
> +
> +      // Remove "inline"
> +      let inline = _strings.GetStringFromName("rule.sourceInline");
> +      let rx = new RegExp("^" + inline + "\\r?\\n?", "g");
> +      text = text.replace(rx, "");

I think the text replacement should be contained in the else block. If we are copying something from an input node, I don't think we necessarily want to change what could be their expected output.

::: browser/devtools/styleinspector/style-inspector-menu.js
@@ +28,5 @@
> +  this.styleDocument = this.view.styleDocument;
> +  this.styleWindow = this.view.styleWindow;
> +
> +  let {CssRuleView} = require("devtools/styleinspector/rule-view");
> +  this.isRuleView = view instanceof CssRuleView;

Let's pass in a options argument to StyleInspectorMenu that indicates whether or not it is a CssRuleView or not over requiring the rule-view.

StyleInspectorMenu(view, options) {
  this.isRuleView = options.isRuleView;
}

@@ +43,5 @@
> +  this._onCopySelector = this._onCopySelector.bind(this);
> +  this._onCopyColor = this._onCopyColor.bind(this);
> +  this._onCopyImageDataUrl = this._onCopyImageDataUrl.bind(this);
> +  this._onToggleOrigSources = this._onToggleOrigSources.bind(this);
> +  this._onShowMdnDocs = this._onShowMdnDocs.bind(this);

Perhaps sort this group of function.bind in alphabetical order.

@@ +65,5 @@
> +    }
> +    popupset.appendChild(this._menupopup);
> +  },
> +
> +  show: function(event) {

Add jsdoc for show(). Something like: "Display the context menu for the style inspector"

@@ +77,5 @@
> +      console.error(e);
> +    }
> +  },
> +
> +  addToView: function() {

I think createContextMenuItems would be a better name. Since we are calling addToView in both rule-view and computed-view, we should remove that extra step in those view and call this method when we initialize StyleInspectorMenu right after this._build().

@@ +256,5 @@
> +
> +    return hasTextSelected;
> +  },
> +
> +  getNodeInfo: function() {

Add jsdoc for getNodeInfo: "Get the type of a given node"

@@ +289,5 @@
> +    this._colorToCopy = container.dataset.color;
> +    return true;
> +  },
> +
> +  _isOnPropertyName: function() {

I think instead of _isOnX we should just keep the helper method named _isX.

	Gabriel [:gl] (ΦωΦ)
	Comment 45 • 9 years ago

Comment on attachment 8630116 [details] [diff] [review]
Bug1164343-part2.v1.patch

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

This looks good! Please just remove that 1 extra space in browser_computedview_matched-selectors_02.js

::: browser/devtools/styleinspector/test/browser_computedview_matched-selectors_02.js
@@ +23,5 @@
>    info("Expanding the matched selectors");
>    propertyView.matchedExpanded = true;
>    yield propertyView.refreshMatchedSelectors();
>  
> +

Remove extra space

	Gabriel [:gl] (ΦωΦ)
	Comment 46 • 9 years ago

Let's keep the 2 parts separate when we land the patches. I have some suggestion on what you could say in the description:
(1) Bug 1164343: Part 1 - Extract context-menu logic from rule-view and computed-view to style-inspector-menu r=mikeratcliffe
(2) Bug 1164343: Part 2 - Refactor style inspector tests to point to the new context-menu and refactor doc.defaultView to styleWindow r=mikeratcliffe

It is important to also include your patch reviewer in the commit description. I am not actually a review peer, but Mike will be able to stamp the final R+. So, please flag him for r? after addressing the feedback.

	Julian Descottes Assignee
	Comment 47 • 9 years ago

Thanks for the review ! 

I am updating the patch with your feedback after a new rebase. 

>> -      let text;
>> -
>> -      if (event.target.nodeName === "menuitem") {
>> -        target = this.doc.popupNode;
>> -      }
> 
> I am unsure about this removal, but maybe Mike might have a 
> better idea why this was needed originally.

This check + reassignment was there to handle copy events coming from the context menu. This is now handled in style-inspector-menu.js. If a copy is triggered from there, the menu calls the current view "copy" method, with the popupNode as parameter. 

> Let's keep the 2 parts separate when we land the patches 

The part1 will fail the tests if part2 is not committed as well. This is not blocking ?

	Gabriel [:gl] (ΦωΦ)
	Comment 48 • 9 years ago

(In reply to Julian Descottes from comment #47)
> Thanks for the review ! 
> 
> I am updating the patch with your feedback after a new rebase. 
> 
> >> -      let text;
> >> -
> >> -      if (event.target.nodeName === "menuitem") {
> >> -        target = this.doc.popupNode;
> >> -      }
> > 
> > I am unsure about this removal, but maybe Mike might have a 
> > better idea why this was needed originally.
> 
> This check + reassignment was there to handle copy events coming from the
> context menu. This is now handled in style-inspector-menu.js. If a copy is
> triggered from there, the menu calls the current view "copy" method, with
> the popupNode as parameter. 
>

Thanks for the insight! Changes LGTM.
 
> > Let's keep the 2 parts separate when we land the patches 
> 
> The part1 will fail the tests if part2 is not committed as well. This is not
> blocking ?

When we commit the changes, we would apply both parts and commit them together in a batch.

You can feedback? me for the changes based on the previous review, and review? mike for the final approval. I think it is very close to shipping. I would note you will want to make changes to the existing patches and resubmit it. This way we can easily just diff the new patch with the previous patch and make sure the new changes in the diff matches what we are expecting from the comments in the review. When you resubmit your patches, you might want to add a [1.0], [2.0], .., [n] or v1, v2, etc to version control your patches in some sense.

	Julian Descottes Assignee
	Comment 49 • 9 years ago

This patch contains the extraction of the context menu logic to style-inspector-menu.js. Also integrated most of the feedback changes suggested by Mike and Gabriel.

Mochitest fixes are in an upcoming patch

	Julian Descottes Assignee
	Comment 50 • 9 years ago

Mochitest fixes for the previous patch

	Gabriel [:gl] (ΦωΦ)
	Comment 51 • 9 years ago

Comment on attachment 8632985 [details] [diff] [review]
Bug1164343-part1.v2.patch

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

Just a fly by review. Overall, very happy with the changes. Just some minor nits.

::: browser/devtools/styleinspector/computed-view.js
@@ +198,4 @@
>    this.createStyleViews();
>  
> +  this._contextmenu = new StyleInspectorMenu(this, {
> +    "isRuleView" : false

This could be { isRuleView: false }

@@ +643,4 @@
>        mozProps.sort());
>  
>      this._createPropertyViews().then(null, e => {
> +      if (!this.destroyed) {

I think we should still keep this as this.styleInspector instead of maintaining this.destroyed state.

::: browser/devtools/styleinspector/rule-view.js
@@ +1383,5 @@
>                 classes.contains("ruleview-selector-matched") ||
>                 classes.contains("ruleview-selector")) {
>        type = overlays.VIEW_NODE_SELECTOR_TYPE;
> +      let ruleEditor = this._getRuleEditorForNode(node);
> +      value = ruleEditor.selectorText.textContent;

Rather than initializing a variable (ruleEditor) that is not used, we can simply do this._getRuleEditorForNode(node).selectorText.textContent

@@ +1392,1 @@
>        let rule = ruleEditor.rule;

We can simply do this._getRuleEditorForNode(node).rule here

@@ +1443,5 @@
>          let count = end - start;
>          text = target.value.substr(start, count);
>        } else {
> +        let win = this.styleDocument.defaultView;
> +        text = win.getSelection().toString();

We can use this.styleWindow.getSelection().toString() here.

	Gabriel [:gl] (ΦωΦ)
	Comment 52 • 9 years ago

Comment on attachment 8632987 [details] [diff] [review]
Bug1164343-part2.v2.patch

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

::: browser/devtools/styleinspector/test/browser_computedview_matched-selectors_02.js
@@ +23,5 @@
>    info("Expanding the matched selectors");
>    propertyView.matchedExpanded = true;
>    yield propertyView.refreshMatchedSelectors();
>  
> +

Remove the extra new line

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 53 • 9 years ago

Comment on attachment 8632985 [details] [diff] [review]
Bug1164343-part1.v2.patch

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

Apart from Gabriel's comments I only found an unneeded Cu.import.

r+ on the condition that you address the nits.

::: browser/devtools/styleinspector/computed-view.js
@@ +23,4 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/devtools/Templater.jsm");

I don't believe you use Templater anywhere in this file so you don't need this import.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 54 • 9 years ago

Also, are you able to push to try? If not then feel free to ping either Gabriel or myself and we can up to try for you.

	Julian Descottes Assignee
	Comment 55 • 9 years ago

> Also, are you able to push to try? 

No I can't push to try yet, would be great if you could submit it for me.
I will request access as Gabriel suggested, but I didn't have time to do so yet.

> r+ on the condition that you address the nits.

Will do !

It feels weird to keep "this.styleInspector" as it's only used as a "isDestroyed" flag.
But everyone else seems to be in favor of it, so I will change it :)

	Gabriel [:gl] (ΦωΦ)
	Comment 56 • 9 years ago

(In reply to Julian Descottes from comment #55)
> > Also, are you able to push to try? 
> 
> No I can't push to try yet, would be great if you could submit it for me.
> I will request access as Gabriel suggested, but I didn't have time to do so
> yet.
> 
> > r+ on the condition that you address the nits.
> 
> Will do !
> 
> It feels weird to keep "this.styleInspector" as it's only used as a
> "isDestroyed" flag.
> But everyone else seems to be in favor of it, so I will change it :)

Hi Julian,

I can push your patch to try while you go through the process of obtaining try access. You can needinfo? me or mike to push to try when you have an update patch that addresses all the comment. That way once the try push test passes, we can move immediately to committing your changes.

	Julian Descottes Assignee
	Comment 57 • 9 years ago

I added back this.styleInspector on the computed view instance as requested. To do so I had to revert back the constructor to expect a styleInspector argument instead of an inspector (styleInspector.inspector) and a document.

Since the initial goal was also to improve consistency between the computedView and the ruleView, I updated the ruleView constructor to use a styleInspector argument as well.

Also updated the destroy methods of both views to nullify the styleInspector, inspector, styleDocument, styleWindow properties.

	Julian Descottes Assignee
	Comment 59 • 9 years ago

@Gabriel : Could you submit the latest patches to try ?

	Gabriel [:gl] (ΦωΦ)
	Comment 60 • 9 years ago

Comment on attachment 8634427 [details] [diff] [review]
Bug1164343-part1.v3.patch

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

::: browser/devtools/styleinspector/style-inspector.js
@@ +25,2 @@
>  
> +  this.view = new RuleView.CssRuleView(this);

Looking deeper into the constructors of CssRuleView and CssComputedView, I am no longer in favor of passing in the StyleInspector object (this). You were right with your changes in part 2 for the arguments passed into these constructors and it looked a lot nicer and cleaner over having a variable (this.styleInspector) that was hardly used.

Can you revert the changes to the CssRuleView and CssComputedView back to what you had in v2? In computed-view.js instead of this.destroyed rename that to this._isDestroyed. I noticed this pattern in the netmonitor-view.js. Sorry for the inconvenience!

You can carry over the r+ from mike since the follow up changes are very minor and does not need another review.

	Julian Descottes Assignee
	Comment 63 • 9 years ago

Just reverted the changes back to what was in v2 (using _isDestroyed instead of destroyed to be consistent with netmonitor-view, as suggested).

Should now be ready for a try push !

	Gabriel [:gl] (ΦωΦ)
	Comment 64 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce9caca7e7e

	Julian Descottes Assignee
	Comment 65 • 9 years ago

Thanks !

A few intermittent test failures but otherwise looks ok.

	Gabriel [:gl] (ΦωΦ)
	Comment 66 • 9 years ago

Looks good Julian! I will land the patch once the tree comes back up. I mention your work during the devtools meeting as well. Thanks for your hard work!

Since this bug is getting quite big, let's move the refactoring of:
(1) ElementStyles
(2) TextPropertyEditor
(3) TextProperty
(4) Rule
into a new bug or their own separate bug to keep that logical separation. We still want to refactor each one separately to make it a speedy review.

	Gabriel [:gl] (ΦωΦ)
	Comment 67 • 9 years ago

We might also want to do a eslint clean of rule-view and computed-view before everything else as well.

	Pulsebot
	Comment 68 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/1991a60abe14
https://hg.mozilla.org/integration/fx-team/rev/7493f7d40868

	Julian Descottes Assignee
	Comment 69 • 9 years ago

Thanks Gabriel ! We finally did it :)

I agree let's move the rest of the refactoring to separate bugs.

Let's split as much as possible : 
- 1 bug for eslint cleanup (for both views)
- 1 bug for extracting ElementStyles
- 1 bug for extracting TextPropertyEditor
- 1 bug for extracting TextProperty
- 1 bug for extracting Rule

I will open those shortly !

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

(In reply to Julian Descottes from comment #69)
> Thanks Gabriel ! We finally did it :)
> 
> I agree let's move the rest of the refactoring to separate bugs.
> 
> Let's split as much as possible : 
> - 1 bug for eslint cleanup (for both views)
> - 1 bug for extracting ElementStyles
> - 1 bug for extracting TextPropertyEditor
> - 1 bug for extracting TextProperty
> - 1 bug for extracting Rule
> 
> I will open those shortly !

Thanks, great to see this landing!

	Julian Descottes Assignee
	Comment 71 • 9 years ago

Opened Bug 1184746 for the eslint cleanup.

	Tim Taubert [:ttaubert] (inactive)
	Comment 72 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1991a60abe14
https://hg.mozilla.org/mozilla-central/rev/7493f7d40868