Closed Bug 1016578 Opened 5 years ago Closed 5 years ago

Enhancement: easily inject jQuery, underscore, or other useful libraries into pages from devtools

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: glind, Assigned: gl)

References

Details

Attachments

(2 files, 8 obsolete files)

proposal:  Surface "injecting" common libraries into content from devtools (toolbar) or cli

Use case:  interacting with, and scraping from "wild" web-content.

Why include this in devtools?
1.  variation in solution, and robustness of those solutions.
2.  dev friendliness
3.  better inferface:  "inject jquery", "inject underscore", "inject lo-dash" or whatever.  


As per:  http://stackoverflow.com/questions/7474354/include-jquery-in-the-javascript-console
Assignee: nobody → gabriel.luong
Do we even want to have this ?

Was it discussed anywhere ?
Attached patch inject.patch (obsolete) — Splinter Review
This was mentioned on the channel yesterday. Originally, robcee mentioned we should do a dropdown menu, but the gcli proposal doesn't seemed like a bad idea either. I quickly pulled off a prototype with the gcli since it would really get people to use the gcli along with the rest of the devtools. 

Video demo: https://cloudup.com/cGJIv754D1Y

There is some interest to inject common libs. Perhaps others can weight in on this. I can definitely see security being a concern.
OS: Mac OS X → All
Hardware: x86 → All
Comment on attachment 8429910 [details] [diff] [review]
inject.patch

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

::: toolkit/devtools/gcli/commands/inject.js
@@ +12,5 @@
> +    name: "inject",
> +    description: gcli.lookup("injectDesc"),
> +  },
> +  {
> +    name: "inject jQuery",

You could probably do this with a single command:


{
  name: "inject"
  params: [
    {
      name: "library",
      type: "selection",
      lookup: [
        { name: "jquery": value: "//ajax.googleapis.com/ajax/libs/jquery/2.1.0/jquery.min.js" },
        { name: "underscore": value: "..." },
        ...
      ]
    }
  ],
  exec: function(args, context) {
    ...
    s.setAttribute("src", args.library);
    ...
  }
}
Attachment #8429910 - Flags: feedback?
0:  Wow, you people move fast!

1.  Security:  If I want to add jQuery to debug a page, I want to add jQuery.  This is a Dev Story, and sharp edges are expected.

2.  Transparency and configuration idea:  new pref:  `devtools.injection.paths`:  '{"jquery": "some/path", "lodash": "some/path"}'.  There is probably prior art on this from other js ecosystems.  Devs can modify this as another hook in.

3.  Surfacing.  I like a button as well as glci.  In my anecdotal experience, gcli isn't well known, and on OSX it's hard to expose (because it uses a function key).  

4.  Repair:  no 'uninjecting', tough!
(In reply to Gregg Lind (User Research - Test Pilot) from comment #4)
> 3.  Surfacing.  I like a button as well as glci.  In my anecdotal
> experience, gcli isn't well known, and on OSX it's hard to expose (because
> it uses a function key).  

One of the problems is that if we added a button for everything, we'd soon have buttons everywhere and the UI would be unusable, so GCLI is a good way to test something out and we can see how much things like this are used before we add buttons.
I was wondering if the first iteration of inject should simply be to add common libraries to devtools.
Is there a preliminary list of libraries developers would like to utiliize?

Another idea mentioned by pbrosset was to also make the command accept a script URL.

Further down the road I would definitely like to see bgrin's DevTools snippets (https://bgrins.github.io/devtools-snippets/) be integrated. I am not sure if it fits this bug.
Attached patch inject.patch (obsolete) — Splinter Review
Added the params selection based on the feedback. I will need to add back the check for conflict handling for each library.
Attachment #8429910 - Attachment is obsolete: true
Attachment #8429910 - Flags: feedback?
Comment on attachment 8430622 [details] [diff] [review]
inject.patch

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

::: toolkit/devtools/gcli/commands/inject.js
@@ +73,5 @@
> +      newSource.addEventListener("load", function(){
> +        console.log(args.library.name + ' loaded!');
> +      });
> +
> +      document.head.appendChild(newSource);

Commands can return promises, so you could also do this:

    exec: function*(args, context) {
      // ...

      let loadPromise = listenOnce(newSource, "load");
      document.head.appendChild(newSource);

      yield loadPromise;

      return args.library.name + ' loaded!'); // Missing L10n!
    }

The upside to this is that it reads top to bottom, and the output displays to the user not just in the console.

Now that requires a new library function:

    const { listenOnce } = require("devtools/async-utils");

And the bad news is that listenOnce isn't there, but it should be! I've got some code that kind of works, maybe you could re-write this to use "new Promise(resolve, reject)" rather than "promise.defer()"

/**
 * A helper for calling addEventListener and then removeEventListener as soon
 * as the event is called, passing the results on as a promise
 * @param element The DOM element to listen on
 * @param event The name of the event to listen for
 * @param useCapture Should we use the capturing phase?
 * @return A promise resolved with the event object when the event first happens
 */
helpers.listenOnce = function(element, event, useCapture) {
  var deferred = promise.defer();
  var onEvent = function(ev) {
    element.removeEventListener(event, onEvent, useCapture);
    deferred.resolve(ev);
  };
  element.addEventListener(event, onEvent, useCapture);
  return deferred.promise;
};
I know we didn't do this very formally, so this is coming out a bit piecemeal.

1.  Should there be a 'post-install' sort of thing, for example calling 'jQuery.noConflict()'.  I say NO on this.

2.  Should there be any indication that injection happened?  `inject --list`?  

3.  Is there any user-editable ways of adding aliases, etc?  (My earlier suggestion for a pref on this solves that, not that grossly.  `inject --add alias path` as an intercept?)

4.  How do people measure the uptake of these sort of features?
Status: NEW → ASSIGNED
Attached patch inject.patch (obsolete) — Splinter Review
I don't think there is an option in the selection type in the gcli to accept a value outside of the lookup list. To accept a script URL, we may need to split up to 2 different commands ("inject url <scriptURL>", "inject add <alias>"). It is not the best solution, and modifying the selection type or creating a new type may be the way to go. Any thoughts on this, Joe?
Attachment #8430622 - Attachment is obsolete: true
Attachment #8431219 - Flags: feedback?(jwalker)
Comment on attachment 8431219 [details] [diff] [review]
inject.patch

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

::: toolkit/devtools/async-utils.js
@@ +51,5 @@
>      }
>      return promise;
>    };
>  };
> +exports.listenOnce = function listenOnce(element, event, useCapture) {

Please could you add some doc comments for this, and while you're at it we generally leave one blank line between functions.

::: toolkit/devtools/gcli/commands/inject.js
@@ +50,5 @@
> +      let src = args.library.src;
> +
> +      let window = context.environment.window;
> +      let document = context.environment.document;
> +      let console = context.environment.window.console;

We can probably inline or remove most of these, can't we? I don't think window or console is/will be used, and both name and src could probably be inlined and people reading the source would have less to remember when working out what was going on.

@@ +60,5 @@
> +      document.head.appendChild(newSource);
> +
> +      yield loadPromise;
> +
> +      console.log(name + " loaded!");

I don't think we need the message in both place do we?

@@ +61,5 @@
> +
> +      yield loadPromise;
> +
> +      console.log(name + " loaded!");
> +      return name + " loaded!";

This needs l10n.
Attachment #8431219 - Flags: feedback?(jwalker) → feedback+
Attached patch inject.patch (obsolete) — Splinter Review
The new union type is still very rough. Perhaps there is a better way to handle the Selection type, which happens to be a function compare to the other types. 

This was the etherpad where we hacked the union type: https://etherpad.mozilla.org/65C9BDoiMw

(In reply to Joe Walker [:jwalker] from comment #11)
> Comment on attachment 8431219 [details] [diff] [review]
> inject.patch
> 
> Review of attachment 8431219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/async-utils.js
> @@ +51,5 @@
> >      }
> >      return promise;
> >    };
> >  };
> > +exports.listenOnce = function listenOnce(element, event, useCapture) {
> 
> Please could you add some doc comments for this, and while you're at it we
> generally leave one blank line between functions.
> 

Fixed. Added doc comments. Surprisingly there was actually a line break between
functions, but that did not show up.

> ::: toolkit/devtools/gcli/commands/inject.js
> @@ +50,5 @@
> > +      let src = args.library.src;
> > +
> > +      let window = context.environment.window;
> > +      let document = context.environment.document;
> > +      let console = context.environment.window.console;
> 
> We can probably inline or remove most of these, can't we? I don't think
> window or console is/will be used, and both name and src could probably be
> inlined and people reading the source would have less to remember when
> working out what was going on.
> 

Fixed. Removed window and console. I kept both name and src as-is due to the
added complexity.

> @@ +60,5 @@
> > +      document.head.appendChild(newSource);
> > +
> > +      yield loadPromise;
> > +
> > +      console.log(name + " loaded!");
> 
> I don't think we need the message in both place do we?
> 

Fixed. Removed the duplicate log messages.

> @@ +61,5 @@
> > +
> > +      yield loadPromise;
> > +
> > +      console.log(name + " loaded!");
> > +      return name + " loaded!";
> 
> This needs l10n.

Fixed. Added gcli.lookup
Attachment #8431219 - Attachment is obsolete: true
Attachment #8432360 - Flags: feedback?(jwalker)
Comment on attachment 8432360 [details] [diff] [review]
inject.patch

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

Looking good.

We're also going to need tests. The cookie tests [1] are a good example, and there are docs on what the properties mean in helpers.js [2]

[1]: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/test/browser_cmd_cookie.js
[2]: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/test/helpers.js#1110

::: browser/locales/en-US/chrome/browser/devtools/gcli.properties
@@ +131,5 @@
>  typesDateMax=%1$S is later than maximum allowed: %2$S.
>  typesDateMin=%1$S is earlier than minimum allowed: %2$S.
>  # LOCALIZATION NOTE: This error message is displayed when the command line is
> +# cannot find a match for the parse types.
> +typesParseNoMatch=No types matched from parsing.

I'm not sure where we might see this string in the UI. We really shouldn't see it at all - what has the user done to deserve this?!

I think we should choose something more generic. It seems likely that 'types' won't mean anything to the user. How about simply 'Command line parsing error'.

@@ +217,1 @@
>  # alias for the remote server (think of it as a "connection name"), and it

I can't see the difference between these 2 lines at all. Can you? Maybe another splinter bug.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +1392,5 @@
> +# parameters.
> +injectDesc=Inject common libraries into the page
> +injectManual=Inject common libraries into the content of the page which can also be accessed from the Firefox console.
> +injectLibraryDesc=Select the library to inject
> +injectLoaded=%1$S loaded!

I don't think we need the '!'. It shouldn't be a surprise that it worked ;-)

::: toolkit/devtools/gcli/source/lib/gcli/types/union.js
@@ +33,5 @@
> +
> +      // Collect the type information and set the type prototype to the
> +      // prototype of the central type if it is a function
> +      this.types = this.types.map(function(type) {
> +        var itemType = centralTypes.getType(type);

I think you'll need 'createType' here so the properties get copied over and the constructor called properly.

@@ +78,5 @@
> +
> +          try {
> +            if (typeof type === "function") {
> +              type.prototype.parse.call(self.types[i], arg, context).then(
> +                function(conversion) {

If we used a => here and below, we could get rid of the 'self' because => auto-bind()s

@@ +82,5 @@
> +                function(conversion) {
> +                  if (conversion.getStatus() === Status.VALID ||
> +                      conversion.getStatus() === Status.INCOMPLETE) {
> +                    self.workingType = self.types[i];
> +                    resolve(conversion);

How about the 'value' of a union type isn't just the simple value, but an object like this:

    {
      type: this.types[i].name,
      'this.types[i].name': value
    }

Which in our case would be:

    {
      type: 'selection',
      selection: {
        name: "lodash",
        src: "http://cdnjs.cloudflare.com/ajax/libs/lodash.js/2.4.1/lodash.min.js"
      }
    }

Or

    {
      type: 'string',
      string: "http://exmaple.com/thing.js"
    }

This would mean 2 things:

1. stringify wouldn't have to guess. It could use the value of 'type'. Yeay! \o/
2. inject.js would have a better way to know what it was working with other than "let name = args.library.name || args.library;"

Instead it would do:

    let name = (args.library.type === 'selection') ? args.library.selection.name : args.library.string;

What do you think?
Attachment #8432360 - Flags: feedback?(jwalker) → feedback+
Attached patch inject.patch (obsolete) — Splinter Review
(In reply to Joe Walker [:jwalker] from comment #13)
> Comment on attachment 8432360 [details] [diff] [review]
> inject.patch
> 
> Review of attachment 8432360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good.
> 
> We're also going to need tests. The cookie tests [1] are a good example, and
> there are docs on what the properties mean in helpers.js [2]
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/
> test/browser_cmd_cookie.js
> [2]:
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/
> test/helpers.js#1110
> 

Fixed. Added unit tests. I know browser_gcli_types.js fail because of TypeError: this.types is undefined.

I haven't looked to see how to fix this yet.

> ::: browser/locales/en-US/chrome/browser/devtools/gcli.properties
> @@ +131,5 @@
> >  typesDateMax=%1$S is later than maximum allowed: %2$S.
> >  typesDateMin=%1$S is earlier than minimum allowed: %2$S.
> >  # LOCALIZATION NOTE: This error message is displayed when the command line is
> > +# cannot find a match for the parse types.
> > +typesParseNoMatch=No types matched from parsing.
> 
> I'm not sure where we might see this string in the UI. We really shouldn't
> see it at all - what has the user done to deserve this?!
> 
> I think we should choose something more generic. It seems likely that
> 'types' won't mean anything to the user. How about simply 'Command line
> parsing error'.
> 

Fixed. Changed the error message to 'Command line parsing error'.

> @@ +217,1 @@
> >  # alias for the remote server (think of it as a "connection name"), and it
> 
> I can't see the difference between these 2 lines at all. Can you? Maybe
> another splinter bug.
> 

This was actually a whitespace that was trimmed. I removed it from the patch since it would otherwise clutter it.

> ::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
> @@ +1392,5 @@
> > +# parameters.
> > +injectDesc=Inject common libraries into the page
> > +injectManual=Inject common libraries into the content of the page which can also be accessed from the Firefox console.
> > +injectLibraryDesc=Select the library to inject
> > +injectLoaded=%1$S loaded!
> 
> I don't think we need the '!'. It shouldn't be a surprise that it worked ;-)
> 

Fixed. Removed the '!'.

> ::: toolkit/devtools/gcli/source/lib/gcli/types/union.js
> @@ +33,5 @@
> > +
> > +      // Collect the type information and set the type prototype to the
> > +      // prototype of the central type if it is a function
> > +      this.types = this.types.map(function(type) {
> > +        var itemType = centralTypes.getType(type);
> 
> I think you'll need 'createType' here so the properties get copied over and
> the constructor called properly.
> 

Fixed. Used createType and removed getType.

> @@ +78,5 @@
> > +
> > +          try {
> > +            if (typeof type === "function") {
> > +              type.prototype.parse.call(self.types[i], arg, context).then(
> > +                function(conversion) {
> 
> If we used a => here and below, we could get rid of the 'self' because =>
> auto-bind()s
> 

Fixed. Arrow functions to the rescue!

> @@ +82,5 @@
> > +                function(conversion) {
> > +                  if (conversion.getStatus() === Status.VALID ||
> > +                      conversion.getStatus() === Status.INCOMPLETE) {
> > +                    self.workingType = self.types[i];
> > +                    resolve(conversion);
> 
> How about the 'value' of a union type isn't just the simple value, but an
> object like this:
> 
>     {
>       type: this.types[i].name,
>       'this.types[i].name': value
>     }
> 
> Which in our case would be:
> 
>     {
>       type: 'selection',
>       selection: {
>         name: "lodash",
>         src:
> "http://cdnjs.cloudflare.com/ajax/libs/lodash.js/2.4.1/lodash.min.js"
>       }
>     }
> 
> Or
> 
>     {
>       type: 'string',
>       string: "http://exmaple.com/thing.js"
>     }
> 
> This would mean 2 things:
> 
> 1. stringify wouldn't have to guess. It could use the value of 'type'. Yeay!
> \o/
> 2. inject.js would have a better way to know what it was working with other
> than "let name = args.library.name || args.library;"
> 
> Instead it would do:
> 
>     let name = (args.library.type === 'selection') ?
> args.library.selection.name : args.library.string;
> 
> What do you think?

I tried the proposed suggestion, and liked it since it would've meant the code was self-documenting, but since the union type can allow for a combination of other types (other than Selection and String), it would be best to keep it a bit more generic. Otherwise, we would have to check for every type. Instead, I modified the Conversion value to { type: this.types[i].name, data: value }.

This meant that I didn't have to check if the type is a selection or not during stringify and just pass in the data values as usual.
Attachment #8432360 - Attachment is obsolete: true
Attachment #8434355 - Flags: review?(jwalker)
Comment on attachment 8434355 [details] [diff] [review]
inject.patch

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

::: toolkit/devtools/gcli/commands/inject.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +const { Cc, Ci, Cu } = require("chrome");
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm");

I think we can virtually always s/Cu.import/require/g in devtools code now. Certainly here we can. Which means we can lose the require(chrome) too.

@@ +53,5 @@
> +      description: gcli.lookup("injectLibraryDesc")
> +    }],
> +    exec: function*(args, context) {
> +      let name = (args.library.type === "selection") ?
> +        args.library.data.name : args.library.data;

Please could you double indent for continuation lines?

  if (buffer.length > foo.wibble.dataReadSoFar &&
    dataWaiting = foo.wibble.inputStream.dataWaiting()) {
    doRead();
  }

You can't tell what this does without reading the line endings quite carefully, which if you have a small screen or a large screen placed vertially, could mean scrolling. But if you double indent for continuation lines it would be much clearer.

@@ +72,5 @@
> +        yield loadPromise;
> +
> +        return gcli.lookupFormat("injectLoaded", [name]);
> +      } catch(e) {
> +        return gcli.lookupFormat("injectFailed", [name]);

Could we put the try/catch just around the call to newURI? Otherwise the error message could be wrong.

::: toolkit/devtools/gcli/source/lib/gcli/types/union.js
@@ +40,5 @@
> +      this.workingType = null;
> +    },
> +
> +    stringify: function(value, context) {
> +      return this.workingType.stringify(value.data, context);

I don't think we need workingType at all any more because value.type tells us what the type is!

@@ +72,5 @@
> +                // data associated with it
> +                if (conversion.value) {
> +                  conversion.value = {
> +                    type: type.name,
> +                    data: conversion.value

I'm going back and forwards on the name for 'data'.

'value' seems like it's more in keeping with the rest of GCLI. For example in inject.js we have: { name: "jQuery", value: ... }. Although that means you have value.value.

The advantage of using the type name as originally proposed is that it forces you to consider the type in question. i.e. no automatic type conversions. You might have to do a little dance with "newConversion.value[type.name] = oldConversion.value;" or something.
Attachment #8434355 - Flags: review?(jwalker) → review+
Attached patch inject.patch (obsolete) — Splinter Review
(In reply to Joe Walker [:jwalker] from comment #16)
> Comment on attachment 8434355 [details] [diff] [review]
> inject.patch
>
> Review of attachment 8434355 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: toolkit/devtools/gcli/commands/inject.js
> @@ +4,5 @@
> > +
> > +"use strict";
> > +
> > +const { Cc, Ci, Cu } = require("chrome");
> > +const { Services } = Cu.import("resource://gre/modules/Services.jsm");
>
> I think we can virtually always s/Cu.import/require/g in devtools code now.
> Certainly here we can. Which means we can lose the require(chrome) too.
>

Fixed. Used require on Services and got rid of require(chrome).

> @@ +53,5 @@
> > +      description: gcli.lookup("injectLibraryDesc")
> > +    }],
> > +    exec: function*(args, context) {
> > +      let name = (args.library.type === "selection") ?
> > +        args.library.data.name : args.library.data;
>
> Please could you double indent for continuation lines?
>
>   if (buffer.length > foo.wibble.dataReadSoFar &&
>     dataWaiting = foo.wibble.inputStream.dataWaiting()) {
>     doRead();
>   }
>
> You can't tell what this does without reading the line endings quite
> carefully, which if you have a small screen or a large screen placed
> vertially, could mean scrolling. But if you double indent for continuation
> lines it would be much clearer.
>

Fixed. Added double indentation. Appreciate the tip!

> @@ +72,5 @@
> > +        yield loadPromise;
> > +
> > +        return gcli.lookupFormat("injectLoaded", [name]);
> > +      } catch(e) {
> > +        return gcli.lookupFormat("injectFailed", [name]);
>
> Could we put the try/catch just around the call to newURI? Otherwise the
> error message could be wrong.
>

Fixed. Added try/catch just around newURI.

> ::: toolkit/devtools/gcli/source/lib/gcli/types/union.js
> @@ +40,5 @@
> > +      this.workingType = null;
> > +    },
> > +
> > +    stringify: function(value, context) {
> > +      return this.workingType.stringify(value.data, context);
>
> I don't think we need workingType at all any more because value.type tells
> us what the type is!
>

Fixed. Removed workingType and perform an Array.find for the type in the stringify function.

> @@ +72,5 @@
> > +                // data associated with it
> > +                if (conversion.value) {
> > +                  conversion.value = {
> > +                    type: type.name,
> > +                    data: conversion.value
>
> I'm going back and forwards on the name for 'data'.
>
> 'value' seems like it's more in keeping with the rest of GCLI. For example
> in inject.js we have: { name: "jQuery", value: ... }. Although that means
> you have value.value.
>
> The advantage of using the type name as originally proposed is that it
> forces you to consider the type in question. i.e. no automatic type
> conversions. You might have to do a little dance with
> "newConversion.value[type.name] = oldConversion.value;" or something.

Fixed. Switched to the proposed method.
Attachment #8434355 - Attachment is obsolete: true
Attached patch inject.patch (obsolete) — Splinter Review
- Added unit test for the union type and inject cmd
- Bumped jQuery version to latest 2.1.1
- Regenerated source from gcli. Most of the pref test changes were ignored, but I kept the gcli.properties changes
- Added a check for a null value in union#stringify as a result of the browser_gcli_types test

https://tbpl.mozilla.org/?tree=Try&rev=09ac770cc457
Attachment #8435278 - Attachment is obsolete: true
Attachment #8435891 - Flags: review?(jwalker)
Attached patch inject.patch (obsolete) — Splinter Review
- I did a search of all the gcli.properties, and it seemed subCommandsNone is still in use. Readded subCommandsNone localization text.
Attachment #8435891 - Attachment is obsolete: true
Attachment #8435891 - Flags: review?(jwalker)
Attachment #8435904 - Flags: review?(jwalker)
^ search of all the gcli.properties removed*

https://tbpl.mozilla.org/?tree=Try&rev=a380f88b6af1
Comment on attachment 8435904 [details] [diff] [review]
inject.patch

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

Awesome work. Thanks.

There are a few bugs that we'll have to dance around in some way:
I expect bug 1020222 will clash with your changes to reduce unused strings, but I expect that to be an easy rebase.
Bug 992309 changed GCLI to use ES6/toolkit promises rather than SDK promises, which probably doesn't change this patch, but does explain the Promise vs promise dance that you had to do in the pull request.

The GCLI code in Firefox is based off the 'mozilla' branch of GCLI so it will be a lot simpler if we use the same version of union.js in both this patch and the PR, so maybe you could copy the non-fat-arrow version here? No need to ask for another review when that's done. Push to try and mark it checkin-needed.
Attachment #8435904 - Flags: review?(jwalker) → review+
(In reply to Joe Walker [:jwalker] from comment #21)
> Push to try and mark it checkin-needed.

I see a try has already been done. No need to do another if this is just a rebase.
Attached patch inject.patchSplinter Review
Rebased promise changes and unused strings.

Doing one last try to make sure the promise changes are okay.
https://tbpl.mozilla.org/?tree=Try&rev=94a8d2989b68
Attachment #8435904 - Attachment is obsolete: true
Attachment #8437057 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/98e074863d41
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/98e074863d41
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Didn't we decide to not hard-code Firefox in strings (bug 812762)?

> injectManual=Inject common libraries into the content of the page which can also be accessed from the Firefox console.
(In reply to Francesco Lodolo [:flod] from comment #26)
> Didn't we decide to not hard-code Firefox in strings (bug 812762)?
> 
> > injectManual=Inject common libraries into the content of the page which can also be accessed from the Firefox console.

Reworded the injectManual string.
Attachment #8437724 - Flags: review?(jwalker)
Comment on attachment 8437724 [details] [diff] [review]
injectstring.patch

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

We need to give the key a new name i.e. s/injectManual/injectManual2/g

The reason for this is that our l10n tools for localizers work on the keys, so by giving them a new key, they get to know that there's a change to take into account.
Attachment #8437724 - Flags: review?(jwalker) → review+
Also I suggest we fix this in a separate bug, to help us keep track of things.
Depends on: 1024585
For what it is worth, Firebug embeds an |include| command for its Console:
https://getfirebug.com/wiki/index.php/Include

I think it is practical and should be also included as a command for the Console (whatever the name of the command is).

Florent
Depends on: 1389874
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.