391279 - Allow createInstance to work for web handlers

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Shawn Wilsher :sdwilsh]

The idea is to reduce code duplication here.  We should get a JS module that has an implementation for nsIWebHandlerApp and nsILocalHandlerApp.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Shawn Wilsher :sdwilsh]

requesting blocking because this blocks a blocking bug

Comment 3

[Shawn Wilsher :sdwilsh]

it looks like it works for local ones, not web ones

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

bye bye JS :(

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 275685 [details] [diff] [review]
v2.0

sr=dmose

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 275685 [details] [diff] [review]
v2.0

ack....

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v3.0

third time is the charm right?

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 275711 [details] [diff] [review]
v3.0

Nice work; this makes the code much cleaner.  A few tweaks would be good:

* name the class nsLocalHandlerAppImpl so that it lines up with the mimeinfo class naming

* move the existing file to nsLocalHandlerAppImpl.{h,cpp} for clarity (just doing "cvs remove" and "cvs add" should be fine; I don't think the blame history here is substantial enough to be worth preserving).

* name the values in the setters something more descriptive than aVal.

* does the JS component need to be added to any packaging files, or is that done automagically by the build system?

sr=dmose

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v3.1

sr=dmose
Addresses comments

I'll get the seamonkey and tbird packages files done at checkin

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 276041 [details] [diff] [review]
v3.1

>+function nsWebHandlerApp()
>+{
>+}

>+nsWebHandlerApp.prototype =
>+{

>+  get name()
>+  {
>+    return this._name;
>+  },

Nit: per style elsewhere (although this varies), and as demonstrated in examples in the JavaScript style guide <http://developer.mozilla.org/en/docs/JavaScript_style_guide>, it would be better to place opening braces next to the block identifier rather than on the next line, and, for empty blocks, to place the closing brace next to the opening one, i.e.:

function nsWebHandlerApp() {}

nsWebHandlerApp.prototype = {

  get name() {
    return this._name;
  },

Comment 11

[Shawn Wilsher :sdwilsh]

really?  wow, that really varies throughout the codebase then, doesn't it?  I'll see if biesi has a preference, but in the past, this is what my reviewers have had me done (or haven't said anything about it)

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> Nit: per style elsewhere (although this varies), and as demonstrated in
> examples in the JavaScript style guide
> <http://developer.mozilla.org/en/docs/JavaScript_style_guide>, it would be
> better to place opening braces next to the block identifier rather than on the
> next line, and, for empty blocks, to place the closing brace next to the
> opening one, i.e.:

I agree.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 276041 [details] [diff] [review]
v3.1

nsLocalHandlerAppImpl.cpp

I don't like the "Impl" in filenames very much, it's really superfluous (what else would be in a cpp file?)

So I'd rename this to nsLocalHandlerApp.cpp

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v3.2

for checkin

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v3.3

for checkin.  yay for buildbot try patch finding a missing file in the patch!

Comment 16

[Shawn Wilsher :sdwilsh]

Checking in netwerk/mime/public/nsIMIMEInfo.idl;
new revision: 1.34; previous revision: 1.33
Checking in docshell/build/nsDocShellModule.cpp;
new revision: 1.30; previous revision: 1.29
Checking in uriloader/exthandler/Makefile.in;
new revision: 1.68; previous revision: 1.67
Checking in uriloader/exthandler/nsCExternalHandlerService.idl;
new revision: 1.8; previous revision: 1.7
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
new revision: 1.337; previous revision: 1.336
Removing uriloader/exthandler/nsHandlerAppImpl.cpp;
new revision: delete; previous revision: 1.4
Removing uriloader/exthandler/nsHandlerAppImpl.h;
new revision: delete; previous revision: 1.6
Checking in uriloader/exthandler/nsLocalHandlerApp.cpp;
initial revision: 1.1
Checking in uriloader/exthandler/nsLocalHandlerApp.h;
initial revision: 1.1
Checking in uriloader/exthandler/nsWebHandlerApp.js;
initial revision: 1.1
Checking in uriloader/exthandler/tests/unit/test_handlerService.js;
new revision: 1.9; previous revision: 1.8
Checking in browser/installer/unix/packages-static;
new revision: 1.126; previous revision: 1.125
Checking in browser/installer/windows/packages-static;
new revision: 1.136; previous revision: 1.135
Checking in suite/installer/windows/packages;
new revision: 1.30; previous revision: 1.29
Checking in mail/installer/windows/packages-static;
new revision: 1.70; previous revision: 1.69