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

RESOLVED FIXED in Thunderbird 55.0

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Thunderbird 55.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

2 years ago
+++ 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.
Assignee

Comment 2

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

Comment 3

2 years ago
(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.
Assignee

Comment 5

2 years ago
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?
Assignee

Comment 8

2 years ago
Posted 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)
Assignee

Comment 9

2 years ago
Posted 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...
Assignee

Comment 12

2 years ago
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+
Assignee

Comment 13

2 years ago
Posted 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+
Assignee

Updated

2 years ago
Attachment #8872628 - Attachment is obsolete: true
Attachment #8872628 - Flags: feedback?(acelists)
Assignee

Comment 15

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

Comment 16

2 years ago
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.

Comment 18

2 years ago
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 21

2 years ago
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+
Assignee

Comment 22

2 years ago
Moved include and fixed comments.
Attachment #8872891 - Attachment is obsolete: true
Attachment #8874313 - Flags: review+
Assignee

Comment 23

2 years ago
https://hg.mozilla.org/comm-central/rev/c823bbbed4285550ff7c86a8dc1b05f0a352fdc9
Assignee: nobody → jorgk
Status: NEW → RESOLVED
Closed: 2 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.