298807 - nsIBookmarksService::ResolveKeyword can make a better use of strings

Status: RESOLVED INVALID

(Firefox :: Bookmarks & History, defect)

Description

[Jungshik Shin]

ResolveKeyword is currently declared as

string resolveKeyword(in wstring aName, out wstring aPostData)

What it returns is actually in UTF-8 so that we'd better declare it as

AUTF8String resolveKeyword(in wstring aName, out AString aPostData)

With that, we can avoid ToNewUnicode and ToNewUTF8String in actual
implementations.Moreover, the string conversion across XPConnect will be done
properly instead of 'blind inflation'.

Comment 1

[Jungshik Shin]

Attachment: patch

asking for r

perhaps, we have to change most of string/wstring to A(C)String in
nsIBookmarksService.idl (especially for out parameters and return values)

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 187345 [details] [diff] [review]
patch

r=me

Comment 3

[Jungshik Shin]

Comment on attachment 187345 [details] [diff] [review]
patch

thanks for r. asking for sr.

Comment 4

[[e-mail]]

Comment on attachment 187345 [details] [diff] [review]
patch

Wouldn't nsAString& be better still? Then you could simply return
GetURLFromResource(source, aShortcutURL);

Comment 5

[Jungshik Shin]

(In reply to comment #4)
> (From update of attachment 187345 [details] [diff] [review] [edit])
> Wouldn't nsAString& be better still? Then you could simply return
> GetURLFromResource(source, aShortcutURL);

I debated it myself and left it alone (reminding myself that some people here
prefer UTF-8 to UTF-16 when most of time, we have ASCII). However, if you like
that way, I'm happy to use AString instead of AUTF8String. 

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 187345 [details] [diff] [review] [edit] [edit])
> > Wouldn't nsAString& be better still? Then you could simply return
> > GetURLFromResource(source, aShortcutURL);
> 
> I debated it myself and left it alone (reminding myself that some people here
> prefer UTF-8 to UTF-16 when most of time, we have ASCII). However, if you like
> that way, I'm happy to use AString instead of AUTF8String. 

Neil makes a good point.. also, all of the callers of this are in JS where
everything is UTF16 anyway, so changing it to use AString won't affect any
callers (and will avoid the need for another UTF8->16 conversion).

Comment 7

[Jungshik Shin]

(In reply to comment #6)
> all of the callers of this are in JS where
> everything is UTF16 anyway, so changing it to use AString won't affect any
> callers (and will avoid the need for another UTF8->16 conversion).

I considered that, too... Wasn't it overwhelmingly in favor of AString? It sure
looks that way, but somehow it didn't look that way when I wrote the patch..


|return GetURLFromResource(source, aShortcutURL);| would change the behavior
slightly if GetURLFromResource succeeds while returning an empty string for
aShortcutURL. I guess that never happens and  checking with IsEmpty (after
checking with NS_FAILED) in the current tree is redundant. Unless I'm wrong on
this point, I'll do |return GetURLFromResource(source, aShortcutURL);| as suggested.

Comment 8

[Jungshik Shin]

Attachment: patch (use AString)

now I use AString as suggested.

Comment 9

[[e-mail]]

Comment on attachment 188012 [details] [diff] [review]
patch (use AString)

>-            nsDependentString postDataStr(postDataVal);
>-            *aPostData = ToNewUnicode(postDataStr);
>+            aPostData = postDataVal;
I note that the original author of the code failed to follow XPCOM rules and
only set aPostData when they felt like it...

Comment 10

[Jungshik Shin]

Comment on attachment 188012 [details] [diff] [review]
patch (use AString)

Thanks for sr. transferring r and asking for a.

I'll truncate aPostData in other cases. 

This is a little enhancement in terms of the code quality and I18n-correctness
with little risk.

Comment 11

[Jungshik Shin]

landed on the trunk

Comment 12

[Asaf Romano (gone)]

regression: bug 300170.

Comment 13

[Benjamin Smedberg]

Please back this out for 1.1a2 due to the regressions in bug 300170, unless
there is a very simple and obvious bustage fix.

Comment 14

[Jungshik Shin]

sorry for the regression. It's not obvious why this regressed. For now, I backed
out the patch.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 188012 [details] [diff] [review]
patch (use AString)

Well, this has regressed due to the null checks on the return value of
|ResolveKeyword()| (in browser.js/navigator.js)

That is, you need to replace |if(!foo)| checks to |if (foo == "")| etc.)

The postdata change might be more tricky, at least for JS callers.

Comment 16

[J.P.Klippel]

adding in browser/components/bookmarks/src/nsBookmarksService.cpp:

    if (aShortcutURL.IsEmpty())
        return NS_ERROR_NULL_POINTER;

replacing the removed:

    NS_PRECONDITION(aShortcutURL != nsnull, "null ptr");
    if (! aShortcutURL)
        return NS_ERROR_NULL_POINTER;

removes the regression (bug 300170). 

Comment 17

[J.P.Klippel]

Attachment: enhanced patch (see comment #16)

Comment 18

[Jungshik Shin]

(In reply to comment #15)
> (From update of attachment 188012 [details] [diff] [review] [edit])
> Well, this has regressed due to the null checks on the return value of
> |ResolveKeyword()| (in browser.js/navigator.js)
> 
> That is, you need to replace |if(!foo)| checks to |if (foo == "")| etc.)
> 
> The postdata change might be more tricky, at least for JS callers.

Yup, I was just looking at JS callers and realized that, too.  Thanks, anyway.

Comment 19

[Bill Gianopoulos [:WG9s]]

Did this also cause the issue of not being to open links with target="_content"
from pages bookmarked in the sidebar?  Try the tinderbox sidebar as an example.

Comment 20

[Anbo Motohiko]

(In reply to comment #19)
> Did this also cause the issue of not being to open links with target="_content"
> from pages bookmarked in the sidebar?  Try the tinderbox sidebar as an example.
Maybe.

BuildID 2005070900 (with attachment #188012 [details] [diff] [review]) has the problem
and BuildID 2005070922 (without attachment #188012 [details] [diff] [review]; backed out at 2005-07-09
07:29 PDT) doesn't have it.

Mozilla/5.0 (Windows; U; Win 9x 4.90; ja; rv:1.8b3) Gecko/20050709 Firefox/1.0+

Comment 21

[Marco Bonardo [:mak]]

Places code is different and not going to take further fixes on 2.x or previous