Closed Bug 1368646 Opened 3 years ago Closed 3 years ago

Port bug 1368418 |Remove some unused i18n modules| to mailnews

Categories

(MailNews Core :: Internationalization, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(3 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1368418 +++
I guess you only need to port nsISemanticUnitScanner and its subclass, which is just the first patch of bug 1368418.
Landed this as a bustage fix:
https://hg.mozilla.org/comm-central/rev/a5afc78bc5d86c9d4280f2955369ff82e3fbab9d

Aceman, would you be able to port the removed code to mailnews:
https://hg.mozilla.org/mozilla-central/rev/c14a7823ce37ca55fc78e0d7e970e8ee764fcd80#l5.2

So instead of being busted, I've just broken the Bayesian filter :-(
Flags: needinfo?(acelists)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> I guess you only need to port nsISemanticUnitScanner and its subclass, which
> is just the first patch of bug 1368418.
We just need the Start() and Next() functions? Without all the infrastructure, IDL, etc. right?
Although of course, things like NextWord() come from nsSampleWordBreaker.
(In reply to Jorg K (GMT+2) from comment #3)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> > I guess you only need to port nsISemanticUnitScanner and its subclass, which
> > is just the first patch of bug 1368418.
> We just need the Start() and Next() functions? Without all the
> infrastructure, IDL, etc. right?
> Although of course, things like NextWord() come from nsSampleWordBreaker.

Yeah, that looks like all you would need. And since nsSemanticUnitScanner::Start actually does nothing, and nsSemanticUnitScanner doesn't have any additional member field, you can directly port nsSemanticUnitScanner::Next as a standalone function which takes an nsSampleWordBreaker.
Had to remove the .h file, too:
https://hg.mozilla.org/comm-central/rev/24b21e9126e0d7e37f44313c7f8fea5c52f972bb

Meanwhile I looked at porting this and it's not so easy since we need nsSampleWordBreaker which is not a published M-C class.
I'm not sure what is counted as a published m-c class. It seems to me you can probably get an instance of nsSampleWordBreaker via NS_WBRK_CONTRACTID?
Attached patch 1368646-move-code.patch (obsolete) — Splinter Review
Something like this. Sadly is doesn't compile since GetClass() comes from nsSampleWordBreaker and that's really not available. Or am I missing something?
Attachment #8872590 - Flags: feedback?(xidorn+moz)
Attached patch 1368646-move-code.patch (v2). (obsolete) — Splinter Review
OK, this compiles now. I had to copy stuff from nsSampleWordBreaker.cpp since the removed class nsSemanticUnitScanner inherited from nsSampleWordBreaker which doesn't have a public interface, especially the GetClass() method is protected.

Aceman, if you can come up with something nicer, please let me know.
Attachment #8872590 - Attachment is obsolete: true
Attachment #8872590 - Flags: feedback?(xidorn+moz)
Attachment #8872628 - Flags: feedback?(xidorn+moz)
Attachment #8872628 - Flags: feedback?(acelists)
I guess this would make your porting easier?
Attachment #8872877 - Flags: feedback?(jorgk)
Actually it is not clear to me at all why we need the XPCOM things for word breaker... It seems to me they can just be some free functions...
Comment on attachment 8872877 [details] [diff] [review]
patch for exposing GetClass

Excellent! Many thanks. I'll attach my rebased patch in the next comment.
Attachment #8872877 - Flags: feedback?(jorgk) → feedback+
Attached patch 1368646-move-code.patch (v3). (obsolete) — Splinter Review
Rebased on Xidorn's patch. No more copying of code :-)
Flags: needinfo?(acelists)
Attachment #8872877 - Flags: review?(VYV03354)
Attachment #8872628 - Flags: feedback?(xidorn+moz)
Comment on attachment 8872877 [details] [diff] [review]
patch for exposing GetClass

Excellent.
Attachment #8872877 - Flags: review?(VYV03354) → review+
Attachment #8872628 - Attachment is obsolete: true
Attachment #8872628 - Flags: feedback?(acelists)
Comment on attachment 8872891 [details] [diff] [review]
1368646-move-code.patch (v3).

OK, this is good for review based on the M-C patch.
Attachment #8872891 - Flags: review?(acelists)
Xidorn, can you please get the M-C part landed.
Flags: needinfo?(xidorn+moz)
Not possible at this moment. All trees are closed... I can probably try to land it tomorrow if trees open again.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8564ce92d8
Expose nsSampleWordBreaker::GetClass as a public static function in nsIWordBreaker. r=emk
Ok, trees are reopened now. Landed.
Flags: needinfo?(xidorn+moz)
Keywords: leave-open
Comment on attachment 8872891 [details] [diff] [review]
1368646-move-code.patch (v3).

Review of attachment 8872891 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.h
@@ +15,5 @@
>  #include "nsString.h"
>  #include "nsWeakReference.h"
>  #include "nsIObserver.h"
> +#include "nsIWordBreaker.h"
> +#include "nsLWBrkCIID.h"

The CIID file does not seem needed here. Move it to the .cpp file.
Attachment #8872891 - Flags: review?(acelists) → review+
Moved include and fixed comments.
Attachment #8872891 - Attachment is obsolete: true
Attachment #8874313 - Flags: review+
https://hg.mozilla.org/comm-central/rev/c823bbbed4285550ff7c86a8dc1b05f0a352fdc9
Assignee: nobody → jorgk
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
You need to log in before you can comment on or make changes to this bug.