Start using the jetpack loader in devtools

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

(Blocks: 3 bugs)

Trunk
Firefox 23
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 730990 [details] [diff] [review]
WIP

This patch starts the process of moving our Cu.import-loaded modules to jetpack-loaded modules.  The long-term benefit is that it lets us more easily load devtools separately from Firefox, for lightweight hacking on the tools without a complete Firefox deve environment.
(Assignee)

Comment 1

5 years ago
Cc-o-rama.

The latest patch converts the inspector and tilt.  The remaining pieces should be easy enough (you'll see in the patch that's a mostly mechanical conversion), but I didn't touch them yet because most of them have entry points other than the toolbox, and that will take a bit of extra thought.

Please take a glance and see if the general idea of it is to your liking.  Everyone should take a look at the parts they care about and raise a flag if there's a problem, but I'm likely to ask for official review from jwalker and one of alex/irakli/mossop.
(Assignee)

Updated

5 years ago
Attachment #730990 - Flags: review?(jwalker)
Comment on attachment 730990 [details] [diff] [review]
WIP

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

I'm insanely happy about this! Let's convert the debugger, too!

::: browser/devtools/framework/gDevTools.jsm
@@ +179,5 @@
>  DevTools.prototype = {
> +  // This is a gross gross hack.  In one place (computed-view.js) we use
> +  // Iterator, but the addon-sdk loader takes Iterator off the global.
> +  // Give computed-view.js a way to get back to the Iterator until we have
> +  // a chance to fix that crap.

I'm curious as to why the sdk loader does that, or is it just a bug?

@@ +436,5 @@
> +    }
> +
> +    // Register the set of default tools
> +    for (let definition of this.Tools.defaultTools) {
> +      this.unregisterTool(definition.id);

Typo in comment.

@@ +457,5 @@
> +
> +    this._chooseProvider();
> +
> +    this._provider.load();
> +    this._newProvider();

this._provider.load() is redundant here, since it's the first thing that _newProvider does.

Also, _chooseProvider seems to always be followed by _newProvider, so you could also add the call to the latter in the former.

::: browser/devtools/framework/Toolbox.jsm
@@ +51,5 @@
>   * @see Consuming promises in https://addons.mozilla.org/en-US/developers/docs/sdk/latest/packages/api-utils/promise.html
>   * @see https://bugzilla.mozilla.org/show_bug.cgi?id=790195
>   * @see https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/promise.js#L179
>   */
> +let promised = (function() {

This can go now if you feel like it, bug 790195 has landed. Promise.all, too.

::: browser/devtools/markupview/Makefile.in
@@ +10,5 @@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
>  libs::
> +	$(NSINSTALL) $(srcdir)/*.js $(FINAL_TARGET)/modules/devtools/markupview

I'm curious about this sort of change: do you have another patch that mechanically copies jsm -> js?
Tilt changes look ok.
Is this patch going to wait on bug 855544 to land or viceversa?
Depends on: 857113
Comment on attachment 730990 [details] [diff] [review]
WIP

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

Except the override manifest hack, I'm really excited to see how simple this patch is!
Do not hesitate to ping us and fill bugs to avoid you any workaround.
Otherwise, have you tried pushing it to try server? (I'm looking forward talos numbers)

::: browser/devtools/framework/gDevTools.jsm
@@ +49,5 @@
> +    delete this.loader;
> +    delete this.require;
> +    delete this.Tools;
> +    delete this.Toolbox;
> +    delete this.TargetFactory;

We may wanna move this SDK-specific code to the loader,
or somehow share this piece of code in order to nuke all compartments created by this destroyed loader:
https://github.com/mozilla/addon-sdk/blob/master/app-extension/bootstrap.js#L314-L326
(Here bootstrap.js isn't shipped into firefox, so that there is no way to share that piece of code)

@@ +69,5 @@
> +        "devtools": "file://" + devtoolsDir,
> +      }
> +    });
> +
> +    this.require = Require(this.loader, { id: "devtools-srcdir" });

This `id` will be printed whenever top-level this.require() calls will fail (most likely when the module won't be found).
It will also be used if any of this call uses a relative path.

Now, I'm not really saying devtools-srcdir is a bad choice.
I just want to highlight why we require an id argument here and what are its usages.

There is also no usage of any main module.
You may gain from this feature if you have anything that looks like a main module. Wouldn't the `DevTools` class be something like this?

@@ +91,5 @@
> +    delete this.require;
> +    delete this.Tools;
> +    delete this.Toolbox;
> +    delete this.TargetFactory;
> +  },

I don't really buy into BuiltinProvider and SrcDirProvider.
this.{require, Tools, Toolbox, TargetFactory} definitions and unload method should definitively be unique/shared.
I would imagine that the only thing that will differ between builtin and external would be the loader path argument and the writeManifest call.

@@ +152,5 @@
> +    }).then(() => {
> +      Components.manager.addBootstrappedManifestLocation(new FileUtils.File(dir));
> +    });
> +  }
> +};

:'( I'm literally crying!!
Do we have a bug filled to implement an api to register new override rules?

Otherwise, here you are not only overriding devtools uri, but all of them, all `content` rules, right?
So that it may also bring easy reload-without-building development to other components!! Do you think it would be possible to filter to only devtools stuff if we want to implement similar feature for let's say browser chrome code?

@@ +179,5 @@
>  DevTools.prototype = {
> +  // This is a gross gross hack.  In one place (computed-view.js) we use
> +  // Iterator, but the addon-sdk loader takes Iterator off the global.
> +  // Give computed-view.js a way to get back to the Iterator until we have
> +  // a chance to fix that crap.

I don't really know why we prevent it from being exposed:
https://github.com/mozilla/addon-sdk/blob/master/lib/toolkit/loader.js#L185
I can understand why we do that for Components and debug, but I would say we can just remove this delete statement and get it back.
--> bug 857113
Attachment #730990 - Flags: feedback+
(Assignee)

Comment 5

5 years ago
(In reply to Alexandre Poirot (:ochameau) from comment #4)
> Comment on attachment 730990 [details] [diff] [review]
> WIP
> 
> Review of attachment 730990 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Except the override manifest hack, I'm really excited to see how simple this
> patch is!

Yeah, will do (see my event target stuff for the first of my pingings).

> Do not hesitate to ping us and fill bugs to avoid you any workaround.
> Otherwise, have you tried pushing it to try server? (I'm looking forward
> talos numbers)

I will push today.

> 
> ::: browser/devtools/framework/gDevTools.jsm
> @@ +49,5 @@
> > +    delete this.loader;
> > +    delete this.require;
> > +    delete this.Tools;
> > +    delete this.Toolbox;
> > +    delete this.TargetFactory;
> 
> We may wanna move this SDK-specific code to the loader,
> or somehow share this piece of code in order to nuke all compartments
> created by this destroyed loader:
> https://github.com/mozilla/addon-sdk/blob/master/app-extension/bootstrap.
> js#L314-L326
> (Here bootstrap.js isn't shipped into firefox, so that there is no way to
> share that piece of code)

Oh yeah, nuking compartments would be great.  Thanks for the pointer.

> 
> @@ +69,5 @@
> > +        "devtools": "file://" + devtoolsDir,
> > +      }
> > +    });
> > +
> > +    this.require = Require(this.loader, { id: "devtools-srcdir" });
> 
> This `id` will be printed whenever top-level this.require() calls will fail
> (most likely when the module won't be found).
> It will also be used if any of this call uses a relative path.
> 
> Now, I'm not really saying devtools-srcdir is a bad choice.
> I just want to highlight why we require an id argument here and what are its
> usages.
> 
> There is also no usage of any main module.
> You may gain from this feature if you have anything that looks like a main
> module. Wouldn't the `DevTools` class be something like this?

Yeah, I think so.  I'm going to rethink the gDevTools/main module/API entry point thing a bit.

> > +    }).then(() => {
> > +      Components.manager.addBootstrappedManifestLocation(new FileUtils.File(dir));
> > +    });
> > +  }
> > +};
> 
> :'( I'm literally crying!!
> Do we have a bug filled to implement an api to register new override rules?
> 
> Otherwise, here you are not only overriding devtools uri, but all of them,
> all `content` rules, right?
> So that it may also bring easy reload-without-building development to other
> components!! Do you think it would be possible to filter to only devtools
> stuff if we want to implement similar feature for let's say browser chrome
> code?

The manifest that it generates is based on the jar.mn, and only overrides specific urls.  So only urls from browser/devtools/jar.mn and toolkit/devtools/jar.mn are overridden.

It really is ugly.

> I don't really know why we prevent it from being exposed:
> https://github.com/mozilla/addon-sdk/blob/master/lib/toolkit/loader.js#L185
> I can understand why we do that for Components and debug, but I would say we
> can just remove this delete statement and get it back.
> --> bug 857113

Thanks.
(Assignee)

Comment 7

5 years ago
(In reply to Victor Porof [:vp] from comment #3)
> Tilt changes look ok.
> Is this patch going to wait on bug 855544 to land or viceversa?

Loser has to rebase. (No specific plan).
(Assignee)

Comment 8

5 years ago
(In reply to Dave Camp (:dcamp) from comment #6)
> https://tbpl.mozilla.org/?tree=Try&rev=03de0f48d91d

That's not going to succeed.  I'll try to get another one up this evening.
Comment on attachment 730990 [details] [diff] [review]
WIP

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

::: browser/devtools/commandline/BuiltinCommands.jsm
@@ +1518,5 @@
> +
> +(function(module) {
> +  gcli.addCommand({
> +    name: "tools",
> +    description: "Commands for managing developer tools.",

Insert discussion of l10n for developers here.

Historically we've l10ned all strings even if hidden behind a chrome pref. But I can see a good argument for not doing that here. The debate about l10n here is similar in some ways to the debate about l10n for doc comments.

@@ +1535,5 @@
> +      }
> +    ],
> +    returnType: "string",
> +    exec: function(args, context) {
> +      let promise = context.createPromise();

We're about to deprecate the old way, in favor of the much more obvious:

    let deferred = context.defer();

::: browser/devtools/inspector/Breadcrumbs.jsm
@@ +49,5 @@
>    this.DOMHelpers = new DOMHelpers(this.chromeWin);
>    this._init();
>  }
>  
> +module.exports = HTMLBreadcrumbs;

Wouldn't:
    exports.HTMLBreadcrumbs = HTMLBreadcrumbs;

Allow for greater flexibility of this module in the future? (Also elsewhere).

::: browser/devtools/styleinspector/CssHtmlTree.jsm
@@ +6,5 @@
>  
> +const {Cc, Ci, Cu} = require("chrome");
> +
> +var ToolDefinitions = require("devtools/framework/tool-definitions");
> +var {CssLogic} = require("devtools/styleinspector/css-logic");

Nit: const > let > var ?

::: browser/devtools/styleinspector/StyleInspector.jsm
@@ +6,5 @@
>  
> +const {Cc, Cu, Ci} = require("chrome");
> +
> +let ToolDefinitions = require("devtools/framework/tool-definitions");
> +let {lazyGetter} = require("devtools/shared/helpers");

I don't see this defined anywhere. Is it in the patch?
Also is 'helpers' a good name given that we're using that in command testing already?

::: browser/devtools/webconsole/test/head.js
@@ +9,5 @@
>  Cu.import("resource://gre/modules/devtools/WebConsoleUtils.jsm", tempScope);
>  let WebConsoleUtils = tempScope.WebConsoleUtils;
>  Cu.import("resource:///modules/devtools/gDevTools.jsm", tempScope);
>  let gDevTools = tempScope.gDevTools;
> +let TargetFactory = gDevTools.TargetFactory;

We talked about having main.TargetFactory or similar. I like that idea. I'm noting it here for Future Generations.

(Hello, Future Generations)
Attachment #730990 - Flags: review?(jwalker) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 733160 [details] [diff] [review]
WIP 2

New version feels a bit better to me:

* devtools/framework/tool-definitions was converted into a main.js.  That just seemed to make sense.  It now also takes care of registering and unregistering the tools it adds to gDevTools itself, rather than gDevTools understanding the defaultTools thing.
* I export a DevToolsModule obj from gDevTools that'll have the properties from main's exports.
* I left in the Iterator hack - it will be easy to remove by rewriting an ugly bit of computed-view.js, but I'd like to do that as a followup.
* i18n for the gcli commands is... started but not complete, and I just remembered that.  Next version will fix.
* Providers are still there, but trimmed down quite a bit.
* instead of helpers.js adding two functions that is used *everywhere* (lazyGetter and lazyImport), I add those to the module globals in main.js.

It's starting to feel kinda nice, I think, though we have a ways to go before it's pretty.

Pushed to try at: https://tbpl.mozilla.org/?tree=Try&rev=9d2010055d71
Attachment #730990 - Attachment is obsolete: true
Attachment #733160 - Flags: review?(jwalker)
Attachment #733160 - Flags: feedback?(poirot.alex)
Comment on attachment 733160 [details] [diff] [review]
WIP 2

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

::: browser/devtools/framework/ToolDefinitions.jsm
@@ +13,5 @@
> +
> +loaderOptions.globals.lazyGetter = XPCOMUtils.defineLazyGetter.bind(XPCOMUtils);
> +loaderOptions.globals.lazyImporter = XPCOMUtils.defineLazyModuleGetter.bind(XPCOMUtils);
> +
> +let {on, off} = require("sdk/system/events");

I added to the devtools thursday meeting, a discussion of

    let module = import(...); module.func(...);

vs:

    let func = import(...).func; func(...);

I'm not proposing that we change this code, but it might be worth deciding if a) we want to all do this the same way and b) which way.

@@ +18,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +
> +lazyGetter(exports, "Toolbox", () => require("devtools/framework/toolbox").Toolbox);

There is an element of surprise here, especially if in another js file: Where does this global come from?

Would it be possible to have "require.lazyGetter"? Then it would obviously come from the host environment.

If not, then perhaps loader.lazyGetter would at least give you a clue, and you'd only have to learn once, not once for each global.

@@ +19,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +
> +lazyGetter(exports, "Toolbox", () => require("devtools/framework/toolbox").Toolbox);
> +lazyGetter(exports, "TargetFactory", () => require("devtools/framework/target").TargetFactory);

And perhaps we could have an API like this:

    lazyModuleGetter(exports, "TargetFactory", "devtools/framework/target");

It seems like that's how it's used mostly.

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +30,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
>                                    "resource://gre/modules/PluralForm.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "DevToolsModule",
> +                                  "resource:///modules/devtools/gDevTools.jsm");

Maybe this is bikeshedding, but I think of things with an initial capital as constructor functions, other things as instances, so it feels like this should be devtoolsModule, or maybe just 'devtools' since we don't have the 'Module' suffix on other modules.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +718,5 @@
>  pagemodRemoveAttributeResult=Elements matched by selector: %1$S. Attributes removed: %2$S.
>  
> +# LOCALIZATION NOTE (toolsDesc) A very short description of the 'tools'
> +# command, the parent command for tool-hacking commands.
> +toolsDesc=Hack the Firefox Developer Tools.

Nit: GCLI descriptions are titles and shouldn't have periods at the end.
Attachment #733160 - Flags: review?(jwalker) → review+
Comment on attachment 733160 [details] [diff] [review]
WIP 2

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

I have the same question than panos about jsm.
I can see you copying js and jsm, but jsm are now used as modules and should be .js.
Like Sidebar.jsm, you do require it with require(devtools/framework/sidebar)
I don't see how it can possibly work with jsm extension.

Also, I don't see any main.js in this patch? Did you forgot putting it in patch or how does that work?

Otherwise, I tried to look at talos results, but it doesn't say much to me:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=6a3cd3b1cc20&newRev=9d2010055d71&submit=true
(I compared to the first revision in history, that had talos tests being run)
It may be easier to land it and look closery at https://areweslimyet.com/ ...?

::: browser/devtools/framework/gDevTools.jsm
@@ +16,5 @@
>  Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
> +
> +let {Loader, Require, main, unload} = Cu.import("resource://gre/modules/commonjs/toolkit/loader.js", {}).Loader;

Here is a case highlighting Joe's comment 11 about how much we should use destructuring.
I have an opinion about this. We should not do that everywhere everytime, especially in big files and for symbols that are very generic.

When you read the code and see "main()" or "unload()" being used, you have absolutely no idea what it actually does nor where is really comes from. You have to search in scope and eventually find it in module header, or not.
You also have main and unload methods on Provider singletons... That's not your fault, this pattern ends up being very common and misleading.

@@ +66,5 @@
> +    return this._writeManifest(devtoolsDir).then((data) => {
> +      this._writeManifest(toolkitDir);
> +    }).then((r) => r, (err) => {
> +      Cu.reportError(err);
> +    });

Wait... what?!
It took me some time to figure out that the comma was actual argument separator.

Couldn't we do .then(null, Cu.reportError); ?
Attachment #733160 - Flags: review+ → review?(jwalker)
Comment on attachment 733160 [details] [diff] [review]
WIP 2

Sorry for the flag reset...

And nevermind my question about jsm and main.js.
It appears that splinter review is really bad with file renaming :/
Attachment #733160 - Flags: review?(jwalker)
Attachment #733160 - Flags: review+
Attachment #733160 - Flags: feedback?(poirot.alex)
Attachment #733160 - Flags: feedback+
(Assignee)

Comment 14

5 years ago
(In reply to Joe Walker [:jwalker] from comment #11)
> > +lazyGetter(exports, "Toolbox", () => require("devtools/framework/toolbox").Toolbox);
> > +lazyGetter(exports, "TargetFactory", () => require("devtools/framework/target").TargetFactory);
> 
> And perhaps we could have an API like this:
> 
>     lazyModuleGetter(exports, "TargetFactory", "devtools/framework/target");
> 
> It seems like that's how it's used mostly.

I have a somewhat sneaky reason for not doing that - the jetpack packager (which we might end up using in some cases) looks for require('..') to decide which things the package might use.
(In reply to Dave Camp (:dcamp) from comment #14)
> I have a somewhat sneaky reason for not doing that - the jetpack packager
> (which we might end up using in some cases) looks for require('..') to
> decide which things the package might use.

Duh, yes of course. That's what the GCLI loader does too.
(Assignee)

Updated

5 years ago
Blocks: 859371
(Assignee)

Updated

5 years ago
Blocks: 859372
(Assignee)

Updated

5 years ago
Blocks: 859380
(Assignee)

Comment 16

5 years ago
Created attachment 736068 [details] [diff] [review]
v3

https://tbpl.mozilla.org/?tree=Try&rev=ad0f96d03241

With the changes joe requested in v2 and a bit of rebasing.
Attachment #733160 - Attachment is obsolete: true
Attachment #736068 - Flags: review?(jwalker)
(Assignee)

Comment 17

5 years ago
Created attachment 736311 [details] [diff] [review]
v4

Fixed netmonitor tests:

https://tbpl.mozilla.org/?tree=Try&rev=38a1304d2be0
Attachment #736068 - Attachment is obsolete: true
Attachment #736068 - Flags: review?(jwalker)
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/549973e335e4
Whiteboard: [fixed-in-fx-team]
(Assignee)

Updated

5 years ago
Blocks: 860917
(Assignee)

Comment 19

5 years ago
Grumble.  Fix incoming.
https://hg.mozilla.org/integration/fx-team/rev/394cd944912b
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 20

5 years ago
Created attachment 736572 [details] [diff] [review]
v4
Attachment #736311 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 736580 [details]
v4.1
Attachment #736572 - Attachment is obsolete: true
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/643194ceabe4

In addition to fixing the failing xpcshell test, I noticed a small MaxHeap regression. I suspect it had to do with devtools.setProvider(), which was forcing lazy initialization of the toolbox and target factory.  I got an r+ from harth and anton for the fix that you can see in the latest patch.
Whiteboard: [fixed-in-fx-team]
It looks like this caused a Trace Mallocs MaxHeap regression on multiple platforms.
(In reply to Matt Brubeck (:mbrubeck) from comment #23)
> It looks like this caused a Trace Mallocs MaxHeap regression on multiple
> platforms.

(but smaller than the MaxHeap regression from the previous patch noted in comment 22, which seems to be partially fixed in the new version)
(Assignee)

Comment 25

5 years ago
Backed out for the MaxHeap regression.  I think I have a fix, but want to land it fresh.
Whiteboard: [fixed-in-fx-team]
Correct me if I'm wrong, but to avoid opening a bug like bug 812762 and that each l10n team working on central has to localize those strings twice, I think you should replace "Firefox" with &brandShortName before landing.

(Hopefully, no one localized those strings before the backout, so you don't have to change the name of the entities
http://mxr.mozilla.org/l10n-central/search?string=toolsManual&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=l10n-central )
(Assignee)

Comment 27

5 years ago
Will do, thanks for the reminder.
(Assignee)

Comment 28

5 years ago
Created attachment 742844 [details] [diff] [review]
v5

Rebased and using observer service manually to avoid pulling in modules at startup.
Attachment #736580 - Attachment is obsolete: true
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/aeb0f8faf02b
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/aeb0f8faf02b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 23
Attachment #742844 - Attachment is patch: true
Attachment #742844 - Attachment mime type: text/x-patch → text/plain
Blocks: 866950
Depends on: 867481
A bit late but I have a couple of notes, not sure if you want a new bug for this.

toolsBuiltinReloaded=Built-in tools loaded.

This is inconsistent with all other strings where you used "builtin". You can fix this without renaming the entity since it's just like fixing a typo.


toolsSrcdirReloaded=Tools loaded from %1$s.
toolsBuiltinReloaded=Built-in tools loaded.
toolsReloaded="Tools reloaded."

Why this difference in structure with strings carrying similar sense?
(In reply to Francesco Lodolo [:flod] from comment #31)
> A bit late but I have a couple of notes, not sure if you want a new bug for
> this.

Anyway, a new bug will be needed because &brandShortName has not been properly replaced in the UI. Dave, you took c#26 literally :)
(In reply to Théo Chevalier [:tchevalier] from comment #32)
> Anyway, a new bug will be needed because &brandShortName has not been
> properly replaced in the UI. Dave, you took c#26 literally :)

I didn't understand your comment, then I realized that's a .properties file
(Assignee)

Comment 34

5 years ago
Yeah, please file a followup.

I really shouldn't be allowed to touch things.

Updated

2 years ago
Blocks: 1192340
You need to log in before you can comment on or make changes to this bug.