Closed Bug 391279 Opened 13 years ago Closed 13 years ago

Allow createInstance to work for web handlers

Categories

(Firefox :: File Handling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 alpha8

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

Details

Attachments

(1 file, 5 obsolete files)

The idea is to reduce code duplication here.  We should get a JS module that has an implementation for nsIWebHandlerApp and nsILocalHandlerApp.
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #275666 - Flags: superreview?(dmose)
requesting blocking because this blocks a blocking bug
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M8
Attachment #275666 - Flags: superreview?(dmose)
it looks like it works for local ones, not web ones
Summary: Get a JavaScript Module to create web and local handler applications → Allow createInstance to work for web handlers
Attached patch v2.0 (obsolete) — Splinter Review
bye bye JS :(
Attachment #275666 - Attachment is obsolete: true
Attachment #275685 - Flags: superreview?(dmose)
Attachment #275685 - Flags: review?(cbiesinger)
Comment on attachment 275685 [details] [diff] [review]
v2.0

sr=dmose
Attachment #275685 - Flags: superreview?(dmose) → superreview+
Comment on attachment 275685 [details] [diff] [review]
v2.0

ack....
Attachment #275685 - Attachment is obsolete: true
Attachment #275685 - Flags: review?(cbiesinger)
Attached patch v3.0 (obsolete) — Splinter Review
third time is the charm right?
Attachment #275711 - Flags: superreview?(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
Attachment #275711 - Flags: superreview?(dmose) → superreview+
Flags: blocking-firefox3? → blocking-firefox3+
Attached patch v3.1 (obsolete) — Splinter Review
sr=dmose
Addresses comments

I'll get the seamonkey and tbird packages files done at checkin
Attachment #275711 - Attachment is obsolete: true
Attachment #276041 - Flags: review?(cbiesinger)
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;
  },
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)
(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 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
Attachment #276041 - Flags: review?(cbiesinger) → review+
Attached patch v3.2 (obsolete) — Splinter Review
for checkin
Attachment #276041 - Attachment is obsolete: true
Attached patch v3.3Splinter Review
for checkin.  yay for buildbot try patch finding a missing file in the patch!
Attachment #276558 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.