Closed Bug 298807 Opened 16 years ago Closed 12 years ago

nsIBookmarksService::ResolveKeyword can make a better use of strings

Categories

(Firefox :: Bookmarks & History, defect)

1.0 Branch
defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: jshin1987, Assigned: jshin1987)

References

Details

Attachments

(2 files, 1 obsolete file)

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'.
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #187345 - Flags: review?(vladimir)
Comment on attachment 187345 [details] [diff] [review]
patch

thanks for r. asking for sr.
Attachment #187345 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 187345 [details] [diff] [review]
patch

Wouldn't nsAString& be better still? Then you could simply return
GetURLFromResource(source, aShortcutURL);
(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. 


Status: NEW → ASSIGNED
(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).
(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.
now I use AString as suggested.
Attachment #187345 - Attachment is obsolete: true
Attachment #188012 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #188012 - Flags: review?(vladimir)
Attachment #187345 - Flags: superreview?(neil.parkwaycc.co.uk)
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...
Attachment #188012 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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.
Attachment #188012 - Flags: review?(vladimir)
Attachment #188012 - Flags: review+
Attachment #188012 - Flags: approval-aviary1.1a2?
Attachment #188012 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
landed on the trunk
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Please back this out for 1.1a2 due to the regressions in bug 300170, unless
there is a very simple and obvious bustage fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
sorry for the regression. It's not obvious why this regressed. For now, I backed
out the patch.
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.
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). 
(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.
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.
(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+
Version: unspecified → 1.0 Branch
Places code is different and not going to take further fixes on 2.x or previous
Status: REOPENED → RESOLVED
Closed: 16 years ago12 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.