270123 - dragging webloc to tab widget "downloads" webloc rather than opening URL

Status: RESOLVED FIXED

(Camino Graveyard :: Drag & Drop, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

2004111108 (v0.8+)

When dragging a webloc file to a tab widget (active or inactive), Camino no
longer goes to the URL specified in the webloc; instead, it attempts to "open"
the file, which in the case of the webloc results in the webloc being "downloaded."

This is a regression from 0.8, where this worked properly (loaded the URL).  I'm
guessing the regression was caused by the fix for bug 243376.  The behavior
worked as expected in the 20041029 NB (I think the last one before bug 243376
was checked in) and fails in 20041104 also (don't remember when the tinderbox
stopped buring, but the tree was closed for a couple of days afer 29-Oct/1-Nov.

Comment 1

[Mike Pinkerton (not reading bugmail)]

-> geoff

Comment 2

[Josh Aas]

Somebody emailed me suggesting that the fix for 243376 caused this.

Comment 3

[Wevah (Nate Weaver)]

(In reply to comment #2)
> Somebody emailed me suggesting that the fix for 243376 caused this.

That's possible, what with the reordering of the pasteboard types. I think we'd
just have to make sure we're detecting .webloc files in the appropriate places.

Comment 4

[Wevah (Nate Weaver)]

Got it; we just need to check for and handle "CorePasteboardFlavorType
0x75726C20" (defined as kCorePasteboardFlavorType_url in NSPasteboard+Utils).

Comment 5

[Josh Aas]

Can somebody please post a webloc file so I can test it on a patch that I got
from Wevah? I don't know how to create them.

Comment 6

[Jasper]

Josh you can create onbe by simply dragging aurl to the desktop.

Comment 7

[Josh Aas]

Attachment: partial fix v1.0

I am not asking for reviews on purpose - this patch does not handle multiple
.webloc files being dragged, and I think it should.

Comment 8

[Josh Aas]

Wevah - do you know how to handle multiple files here?

Comment 9

[Wevah (Nate Weaver)]

(In reply to comment #8)
> Wevah - do you know how to handle multiple files here?

Sure, if the pasteboard flavor returns multiple URLs instead of just the first.
If it doesn't, we'd have to read the .webloc files ourselves to get the URLs,
and since they're in the resource fork, it'll be annoying.

I did just realize that if you drag a normal file and a .webloc, it'll probably
only handle the .webloc. How do we want to handle that?

Comment 10

[Wevah (Nate Weaver)]

Nevermind; I'm going to have to do the manual resource-grabbing anyway.

Comment 11

[Josh Aas]

Attachment: fix v2.0

This seems to do things correctly, and handles not only multiple files but
combinations of different types of files. Patch by Wevah. I am not giving it r+
myself yet. If somebody could check memory management in the .webloc URL
extraction code that would be great. I'm not too familiar with carbon memory
management.

Comment 12

[Wevah (Nate Weaver)]

(In reply to comment #11)
> If somebody could check memory management in the .webloc URL
> extraction code that would be great. I'm not too familiar with carbon memory
> management.

Yeah; I'm not really either, though I did run |leaks| on my test app to check. I
didn't see anything that led me to believe I'd have to free the memory myself,
though.

Comment 13

[Jasper]

Why is this written in Carbon and not Cocoa? Seems a little odd.

Comment 14

[Wevah (Nate Weaver)]

(In reply to comment #13)
> Why is this written in Carbon and not Cocoa? Seems a little odd.

Because .webloc files store their URL data in the resource fork, and Cocoa has
no proper way to read resource fork data.

The reading of the 'url ' resource is the only part of the code that's in
Carbon, though; everything else is written in or coerced to Cocoa stuff.

Comment 15

[Mike Pinkerton (not reading bugmail)]

using carbon is fine for this stuff.

Comment 16

[Josh Aas]

Comment on attachment 168274 [details] [diff] [review]
fix v2.0

Requesting sr as it would be nice to have this landed ASAP so I can fix bug
242934.

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 168274 [details] [diff] [review]
fix v2.0

+NSString *urlStringFromWebloc(NSString *file) {
+  FSRef ref;
+  FSSpec spec;
+  short resRef;
+  Handle urlResHandle;
+  long size;
+  NSString *ret = nil;

Declare these only in the scopes they're needed. This is c++. eg, |resRef|
should be in the scope of the if.

Also, what happens if |file| is nil? Can FSRefMakePath handle that? Also,
please, rename |file| to |inFile|, it's a param.

Braces are on a separate line for functions (nit picky, i know).

+  NSString *urlline, *ret = nil;
+  NSArray *contents = [[NSString stringWithContentsOfFile:file]
componentsSeparatedByString:@"\r\n"];

same comments about scoping, var name, and nil check.

looks reasonable otherwise.

Comment 18

[Wevah (Nate Weaver)]

(In reply to comment #17)
Alright; will fix.

Comment 19

[Josh Aas]

Wevah - how are you making patches? You always have a funny file path (begins
with mozilla/ instead of being inside the tree), and there are Windows
linebreaks in the file. Here's how I recommend doing it:

1. cd mozilla/camino
2. cvs diff -u > ~/Desktop/file.patch

Comment 20

[Wevah (Nate Weaver)]

(In reply to comment #19)
> Wevah - how are you making patches? You always have a funny file path (begins
> with mozilla/ instead of being inside the tree), and there are Windows
> linebreaks in the file. Here's how I recommend doing it:
> 
> 1. cd mozilla/camino
> 2. cvs diff -u > ~/Desktop/file.patch
> 

a) I can't build Camino on my computer. I am running 10.2.8 (yes, I know:
silly), hence why you told me to send the patches to you to test and post,
instead of me posting them myself. I have been grabbing the cvs to my drive and
copying the file. I was unaware (understandably or not) of the 'cvs diff'
method, as I really don't use cvs much. I will however try using that in the future.

b) There shouldn't be Windows linebreaks anywhere, unless BBEdit is screwing
them up somehow (that's what I'm using to paste in the functions from my test 
application for this patch, since I can't compile it in myself). Can you tell me
where they are appearing?

I don't know; maybe I should just hold of on anything until I can update my stuff.

Comment 21

[Wevah (Nate Weaver)]

Oh, right: This is how I'm doing the diffs (wrong, I suppose):

diff -u path/to/oldfile.mm path/to/newfile.mm

I don't see any Windows linebreaks in the patch file (I just looked), so I
really don't have any idea how they're getting in there. :/

Comment 22

[Josh Aas]

Attachment: fix v3.0

Comment 23

[Josh Aas]

Attachment: fix v4.0

Comment 24

[Josh Aas]

landed on trunk

Comment 25

[Josh Aas]

Comment on attachment 169850 [details] [diff] [review]
fix v4.0

got sr on irc