Allow createInstance to work for web handlers

RESOLVED FIXED in Firefox 3 alpha8

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
Firefox 3 alpha8
Points:
---
Bug Flags:
blocking-firefox3 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

11 years ago
The idea is to reduce code duplication here.  We should get a JS module that has an implementation for nsIWebHandlerApp and nsILocalHandlerApp.
(Assignee)

Comment 1

11 years ago
Created attachment 275666 [details] [diff] [review]
v1.0
Attachment #275666 - Flags: superreview?(dmose)
(Assignee)

Comment 2

11 years ago
requesting blocking because this blocks a blocking bug
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M8
(Assignee)

Updated

11 years ago
Attachment #275666 - Flags: superreview?(dmose)
(Assignee)

Comment 3

11 years ago
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
(Assignee)

Comment 4

11 years ago
Created attachment 275685 [details] [diff] [review]
v2.0

bye bye JS :(
Attachment #275666 - Attachment is obsolete: true
Attachment #275685 - Flags: superreview?(dmose)
Attachment #275685 - Flags: review?(cbiesinger)

Comment 5

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

sr=dmose
Attachment #275685 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 6

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

ack....
Attachment #275685 - Attachment is obsolete: true
Attachment #275685 - Flags: review?(cbiesinger)
(Assignee)

Comment 7

11 years ago
Created attachment 275711 [details] [diff] [review]
v3.0

third time is the charm right?
Attachment #275711 - Flags: superreview?(dmose)

Comment 8

11 years ago
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+

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
(Assignee)

Comment 9

11 years ago
Created attachment 276041 [details] [diff] [review]
v3.1

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;
  },
(Assignee)

Comment 11

11 years ago
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+
(Assignee)

Comment 14

11 years ago
Created attachment 276558 [details] [diff] [review]
v3.2

for checkin
Attachment #276041 - Attachment is obsolete: true
(Assignee)

Comment 15

11 years ago
Created attachment 276566 [details] [diff] [review]
v3.3

for checkin.  yay for buildbot try patch finding a missing file in the patch!
Attachment #276558 - Attachment is obsolete: true
(Assignee)

Comment 16

11 years ago
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
Last Resolved: 11 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.