The default bug view has changed. See this FAQ.

separate xpcom wrapper and charset detector

RESOLVED FIXED

Status

()

Core
Internationalization
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: Shanjian Li, Assigned: Jungshik Shin)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

15 years ago
Unlike rest of the universal detector implementation file, nsUniversalDetector.h
and nsUniversalDetector.cpp contains both implementation class (which has no
dependency on XPCOM) and 2 XPCOM wrapper class. Separate those class to
different files will benefit maintenance and future reuse.
(Reporter)

Comment 1

15 years ago
Created attachment 106410 [details] [diff] [review]
patch
(Reporter)

Comment 2

15 years ago
patch
Status: NEW → ASSIGNED
Summary: separate xpcom wrapper and charset detector → separate xpcom wrapper and charset detector

Comment 3

15 years ago
Created attachment 107934 [details] [diff] [review]
Adding the new file to the project.
(Reporter)

Updated

15 years ago
Attachment #106410 - Flags: superreview?(alecf)
Attachment #106410 - Flags: review?(ftang)
(Reporter)

Updated

15 years ago
Attachment #107934 - Flags: superreview?(alecf)
Attachment #107934 - Flags: review?(ftang)

Comment 4

14 years ago
Comment on attachment 106410 [details] [diff] [review]
patch 

- You don't need to NS_INIT_ISUPPORTS anymore
- Why not make mObserver an nsCOMPtr?

with those changes, sr=alecf
Attachment #106410 - Flags: superreview?(alecf) → superreview+

Comment 5

14 years ago
Comment on attachment 107934 [details] [diff] [review]
Adding the new file to the project.

sr=alecf
Attachment #107934 - Flags: superreview?(alecf) → superreview+

Comment 6

13 years ago
CC'ing jungshik to see what if any we should do about this bug.

Comment 7

13 years ago
Well why not just do the check-in ? There will very little update needed as the
code almost didn't change since the patch was written.

And it would make things easier for independant use of the detector, like for
example what I suggested at the end of bug 135762.
(Assignee)

Comment 8

13 years ago
Created attachment 155006 [details] [diff] [review]
patch (addressing alecf's comment and changing license to MPL)

attachment 106410 [details] [diff] [review] needs a little update (it didn't get cleanly applied) in
addition to what alecf's pointed out. attachment 107934 [details] [diff] [review] is not needed any more.
Assignee: shanjian → jshin
Attachment #106410 - Attachment is obsolete: true
Attachment #107934 - Attachment is obsolete: true
(Assignee)

Comment 9

13 years ago
Comment on attachment 155006 [details] [diff] [review]
patch (addressing alecf's comment and changing license to MPL)

asking for r (just in case).

Jean-Marc, thanks for reminding me of this (somehow I don't remember reading
Kat's comment)
Attachment #155006 - Flags: superreview+
Attachment #155006 - Flags: review?(smontagu)
Comment on attachment 155006 [details] [diff] [review]
patch (addressing alecf's comment and changing license to MPL)

>+NS_IMETHODIMP nsUniversalXPCOMDetector::DoIt(
>+  const char* aBuf, PRUint32 aLen, PRBool* oDontFeedMe)
>+{
>+  NS_ASSERTION(mObserver != nsnull , "have not init yet");
>+
>+  if((nsnull == aBuf) || (nsnull == oDontFeedMe))
>+     return NS_ERROR_ILLEGAL_VALUE;
>+
>+  this->HandleData(aBuf, aLen);

Make this:
    nsresult rv = this->HandleData(aBuf, aLen);
    if (NS_FAILED(rv))
      return rv;
(Change from bug 225994)

>+NS_IMETHODIMP nsUniversalXPCOMStringDetector::DoIt(const char* aBuf, PRUint32 aLen, 
>+                     const char** oCharset, nsDetectionConfident &oConf)
>+{
>+   mResult = nsnull;
>+   this->Reset();
>+   this->HandleData(aBuf, aLen); 

Make the corresponding change from bug 225994 here too. Also detabify the new
files and r=smontagu
Attachment #155006 - Flags: review?(smontagu) → review+
(Assignee)

Comment 11

13 years ago
patch checked in with Simon's concerns addressed. I changed the indentation a
bit more.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #106410 - Flags: review?(ftang)

Updated

12 years ago
Attachment #107934 - Flags: review?(ftang)
You need to log in before you can comment on or make changes to this bug.