234936 - Don't inline spell check URLs

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Scott MacGregor]

This patch has been going into the Thunderbird builds since this summer but I
never got it checked into the tree for seamonkey to use to.

It just exposes functionality that already exists in the mozTxtToHTML converter
into a generic method for scanning a text string for a url.

Comment 1

[Scott MacGregor]

Attachment: the patch

Comment 2

[Scott MacGregor]

fixed for seamonkey now.

Comment 3

[Darin Fisher]

uhm... how about a heads-up next time?

Comment 4

[Darin Fisher]

Comment on attachment 141759 [details] [diff] [review]
the patch

>Index: netwerk/streamconv/converters/mozTXTToHTMLConv.cpp

>+NS_IMETHODIMP mozTXTToHTMLConv::FindURLInPlaintext(const PRUnichar * aInString, PRInt32 aInLength, PRInt32 aPos, PRInt32 * aStartPos, PRInt32 * aEndPos)
>+{
>+  // call FindURL on the passed in string
>+  nsString outputHTML; // we'll ignore the generated output HTML

maybe use a nsAutoString instead in the hopes of avoiding a needless heap
allocation?


>Index: netwerk/streamconv/public/mozITXTToHTMLConv.idl

>+  void findURLInPlaintext([const] in wstring text, in long aLength, in long aPos, out long aStartPos, out long aEndPos);

why do you need to declare |text| as const?  it will already be |const| by
virtue of the fact that it is an in-param.  some documentation of this
method would also be nice.  what do the parameters mean?

Comment 5

[Ben Bucksch (:BenB)]

em, yes. This is a dup of bug 172186 together with bug 116242, which I spent a
good day (IIRC) to fix due to mscott's request, and it also contains a number of
other fixes to the converter. He also said there that my patch would be what
should go into the main codebase eventually. I'd appreciate, if that would
actually happen, and I'd also appreciate to be on the hook for changes to the
converter, esp. API-changes.

Could you please back this out and try to get my old patch in? Thanks. REOPENing.

Comment 6

[Scott MacGregor]

sorry Ididn't think anyone cared....

i changed it from an nsAutoString to an nsString because I thought most of the
buffers I was passing in where going to be long.

I'll change it back.

> why do you need to declare |text| as const? 

I was just following the pattern used by every other method in that interface.

See 
wstring   scanTXT([const] in wstring text, in unsigned long whattodo);
and
wstring   scanHTML([const] in wstring text, in unsigned long whattodo);

I'll add some comments

Comment 7

[Ben Bucksch (:BenB)]

FYI, IIRC, the reason why we needed the larger patch was partially explained in
bug 172186 comment 11 (mscott's code would still try to spellcheck
well-delimited URLs like <http://www.bucksch.com> and URLs with punctation like
http://www.bucksch.com, http://www.bucksch.com), and the function is also needed
for "Load URL" context menu in the browser, where the URL is also just a substring.

Comment 8

[Ben Bucksch (:BenB)]

(sorry, hit commit to early)
Oh, and before you're scared about the big patch in bug 116242, I did a number
of other fixes and lots of indention changes while I was at it.

Comment 9

[Darin Fisher]

(In reply to comment #6)
> sorry Ididn't think anyone cared....

no big deal really... i just like to know when API changes go into necko. 
otherwise, i'm caught by surprise if someone starts talking about some API that
i know nothing about but should as the module owner and all ;-)


> i changed it from an nsAutoString to an nsString because I thought most of the
> buffers I was passing in where going to be long.
> 
> I'll change it back.

hmm... that does make sense.  if you want to keep it as nsString for that
reason, i can understand.  perhaps a comment would suffice?  otherwise, some
casual reader might wonder.


> > why do you need to declare |text| as const? 
> 
> I was just following the pattern used by every other method in that interface.
> 
> See 
> wstring   scanTXT([const] in wstring text, in unsigned long whattodo);
> and
> wstring   scanHTML([const] in wstring text, in unsigned long whattodo);
> 
> I'll add some comments

yeah, you're right... the rest of the IDL is equally making a point of declaring
|const| for no good reason.  you know... the problem is that the whole IDL needs
some lov'in :-(

Comment 10

[Ben Bucksch (:BenB)]

I guess I added the const because the function is not supposed to change the
string, and const garantees that to the caller. Is there a reason for it not to
be const? Maybe you should file a new bug about it.

Comment 11

[Scott MacGregor]

Attachment: some IDL love....

Ben, the reason why an explicit const is not neccessary here has to do with the
fact that you declard all of these text parameter as input (in) parameters. The
IDL compiler automoatically translates that to a const string.

Comment 12

[Darin Fisher]

Comment on attachment 141765 [details] [diff] [review]
some IDL love....

sr=darin

Comment 13

[Ben Bucksch (:BenB)]

mscott, OK. So, can we back this out and get my patch (incl. that const change)
in instead, please? Piling up changes on the current code would make that
harder. I'd say spurious spellcheck warnings for <[e-mail]> are more
severe than a spurious |const| ;-).

Comment 14

[OstGote!]

(In reply to comment #5)
> em, yes. This is a dup of bug 172186 together with bug 116242, ...

Comment 15

[Wayne Mery (:wsmwk)]

(In reply to comment #13)
> mscott, OK. So, can we back this out and get my patch (incl. that const change)
> in instead, please? Piling up changes on the current code would make that
> harder. I'd say spurious spellcheck warnings for <[e-mail]> are more
> severe than a spurious |const| ;-).

checkin?  (target is blasted)

Comment 16

[Ria Klaassen (not reading all bugmail)]

*** Bug 344609 has been marked as a duplicate of this bug. ***

Comment 17

[Ria Klaassen (not reading all bugmail)]

Changing product and component because it also affects Firefox now.

Comment 18

[Brett Wilson]

I don't imagine this blocks release. There are two problems:

There is some code in the word finder file to detect URLs. It is currently commented out becuase there is a bug in it somewhere and it crashes. It is probably easy to fix, but I was in a hurry to get the new word finder working.

If this is fixed, the other problem is with highlighting. This problem might be OK to ship with. Say you have "htp://qwert.com" which is an invalid URL. You get 3 misspellings: "htp" "qwert", and "com". When you add a "t", you get a valid URL and we need to delete any marked misspellings in the range.

This is not really supported by the current spellchecker; it just checks the last character to see if it is in the range, and then deletes the range if it is. We could come up with more elaborate ways to check, but this makes it very complicated. Checking ranges is the slowest thing already, and it needs to be much faster for release. Complicating it is very undesirable.

We might be able to ship with the second bug. Just fixing the first might be less annoying than the current behavior.

Comment 19

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

> Say you have "htp://qwert.com" which is an invalid URL.

That depends on your definition of "invalid URL". Per Necko's definition, it's a fine URL.

Comment 20

[David Baron :dbaron:]

Per drivers, doesn't sound like a blocker for 1.8.1, especially given comment 18.

Comment 21

[Brett Wilson]

roc, pkasting, and I discussed this. Once bug 345099 is fixed, we should be able to do much faster range+selection intersections.

Then, we can check for any ranges that intersect the current word and remove all of them. This shouldn't be too hard, I will try to get this working for 2.0 beta 2 if possible.

Comment 22

[Brett Wilson]

*** Bug 343610 has been marked as a duplicate of this bug. ***

Comment 23

[Brett Wilson]

Attachment: Patch

This patch requires the patchs in bug 345099 and bug 345751.

Comment 24

[Brett Wilson]

Fixed on trunk.

Comment 25

[Mike Schroepfer]

Comment on attachment 230821 [details] [diff] [review]
Patch

approved by schrep for drivers

Comment 26

[Brett Wilson]

Fixed on branch.

Comment 28

[Carsten Book [:Tomcat]]

Verified fixed 1.8.1.3 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.3) Gecko/20070326 Thunderbird/2.0.0.0 Mnenhy/0.7.5.0 ID:2007032620 and tested with various urls - no spell check on urls.