Imported NetPositive Bookmarks are not loaded properly anymore

RESOLVED FIXED

Status

()

Core
RDF
--
major
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: Paul, Assigned: Sergei Dolgov)

Tracking

({regression})

Trunk
x86
BeOS
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

16 years ago
Something changed between teh 6th and the 8th of May, that broke the display of imported
Net+ bookmarks.  Here's a list of changes to cvs for that time perios:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=
HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date
=explicit&mindate=5%2F6%2F2002&maxdate=5%2F8%2F2002&cvsroot=%2Fcvsroot

I believe it is something in the xpcom/io directory, but I have not narrowed it down yet.
It should not be hard to do so, however, so, I will hopefully have a fix soon.
(Assignee)

Comment 1

16 years ago
Chris, this problems sees in your competence.
I noticed lot of your checkins in
nsBookmarksService 
and
nsFileSystemDataSource
in beginnin of May 2002, when Net+ bookmarks were broken.
I suspect that reason may be related to all this dance around  UC2/UTF-8
(Assignee)

Comment 2

16 years ago
nsresult FileSystemDataSource::getNetPositiveURL in
rdf/datasource/src/nsFileSystemDataSource.cpp needs rewrote:
1)Check-in for XP_WIN
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/components/bookmarks/src&command=DIFF_FRAMESET&file=nsBookmarksService.cpp&rev1=1.238&rev2=1.239&root=/cvsrootv
breaks BeOS functionality (they even didn't put if define(XP_WIN) in generic code!
2) Even if this check-in is rolle back, there is still problem in
getNetPositiveURL: 

constructor BFile   bf(nativeURI, B_READ_ONLY); gets path in Escaped form, when
it hsould be pure UTF-8 (native BeOS encoding).
I'm confused by your previous post but to clarify the situation, my checkins to
those files were for a) a global obsolete macro cleanup and b) to checkin
Takashi Toyoshima's changes to actually use the netPositive import routines. 
Both of which occurred long before May 2002.  

It does look like the nsIFile changes triggered the problem though.  Cc'ing a
couple of people who would understand it better than I.  The changes that you
pointed out appear to only occur inside a XP_WIN ifdef so I'm confused as to how
they break BeOS.  Or did you mean that we need to make similar changes to the
BeOS routines to correct the problem?
(Assignee)

Comment 4

16 years ago
hey Chris

1)i'm sorry. #endif before :ParseFavoritesFolder closed #if _MSC_VER >= 1200
case, not XP_WIN case, as i thought

2)yeah i mean't that there is need to check effect of Bug 142870 on BeOS. Seems
titanic work, but i have feeling that this is quite important for other BeOS bugs.

3)Coffee instead sleeping is evil - now i had working Net+ bookmarks, but
besides hacks  in FileSystemDataSource::getNetPositiveURL() i don't know now
what rollback certainly helped me to restore its functionality. Will try
diff-ing again this night/
(Assignee)

Comment 5

16 years ago
Created attachment 87536 [details] [diff] [review]
patch #1 - nsBookmarksService.cpp

sets useDynamicSystemBookmarks = PR_TRUE; by default in BeOS
It was broken by adding 
   if (bookmarksPrefs)
     bookmarksPrefs->GetBoolPref("import_system_favorites",
&useDynamicSystemBookmarks);
AFTER #ifdef XP_BEOS
see comments inside patch. Other soultion is follow bug 22642 and create item
in BookmarkManager->View aslo for BeOS build
patch for FileSystemDataSource::getNetPositiveURL() will follow
(Assignee)

Comment 6

16 years ago
second notice about - nsBookmarksService.cpp - CRITICAL!
checking
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/components/bookmarks/src&command=DIFF_FRAMESET&file=nsBookmarksService.cpp&rev1=1.237&rev2=1.238&root=/cvsroot


for bug 129428 is main break reason - he breaks proper Net+ folder scanning by
assigning blindly file///: to all entries in NetPositive/Bookmarks folder, and
preventing from call to GetUrl->GetNetpositiveURL from BFile "META:url" attribute.
I fixed it for BeOS but need contact to above checkin author
Also dependency on bug 129428 must be set

Updated

16 years ago
Severity: normal → major
Depends on: 129428
Keywords: regression
(Assignee)

Comment 7

16 years ago
Created attachment 87612 [details] [diff] [review]
simplest patch

patch.
Attachment #87536 - Attachment is obsolete: true
(Assignee)

Comment 8

16 years ago
additional patch for FileSystemDataSource::getNetPositiveURL
is needed to fix handling of Net+ bookmarks files with non-ASCII-7 names.
(Unescape())
Assignee: arougthopher → sergei_d
(Assignee)

Comment 9

16 years ago
Ben,
can you verify/confirm that patch
http://bugzilla.mozilla.org/attachment.cgi?id=87612&action=view
does not break anything for other platforms? 
(Reporter)

Comment 10

16 years ago
Ok, this seems to work, but, BeOS will now not be able to support the
"import_system_favorites" preference.  Could we not default it to true,
in the default prefs file that is distributed with mozilla for BeOS?
Just an idea.
(Assignee)

Comment 11

16 years ago
hm, problem is that this option in UI exists only in Win/Mac XUL-s/jar-s.
Until someone create proper UI module for BeOS, i prefer to forget about this
option in preferences :) Though, it may be set for BeOS by default in all.js or
BookmarkOverlay.js wherever else. Just in order to inform people about situation.
(Assignee)

Comment 12

16 years ago
Created attachment 87821 [details] [diff] [review]
i18n fix

Allows to get URL from bookmark files with non-ascii characters.
(Assignee)

Comment 13

16 years ago
There is another problem now with N+ bookmarks.
Search don't work for those Net+ Imported bookmarks.
Dunno if it worked before break.
so, BookmarkService needs revisiting, but maybe problem is on JS code.
Paul, can you check it, and what do you think, will it be another bug?
(Reporter)

Comment 14

16 years ago
Question:  Does the search work under the Windows version?  
If not, we probably don't need to worry about it.  Especially since they are
Net+ bookbark files, which are native files under BeOS, and cn be queried on the file
system :)
(Assignee)

Comment 15

16 years ago
Do you mean search in "Imported IE Favorites"?
I cannot check it, because i haven't IE  here (all 3 machines with
Win98Lite/SLEEK). So no subject to import

(Assignee)

Comment 16

16 years ago
*** Bug 157353 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 17

16 years ago
Created attachment 91843 [details] [diff] [review]
Restyled patch for nsBookmarkService
Attachment #87612 - Attachment is obsolete: true
(Assignee)

Comment 18

16 years ago
Comment on attachment 87821 [details] [diff] [review]
i18n fix

Not needed if we commit patch for bug 158092.
That's more general solution.
Attachment #87821 - Attachment is obsolete: true
(Assignee)

Comment 19

16 years ago
Created attachment 91857 [details] [diff] [review]
mv tabs /dev/null

additional cleanup
Attachment #91843 - Attachment is obsolete: true

Comment 20

16 years ago
Comment on attachment 91857 [details] [diff] [review]
mv tabs /dev/null

whitespace nightmares, don't worry, r=timeless
but this:
+	else
+	{
+#if defined(XP_BEOS)
should be
+    else
+    {
+#ifdef XP_BEOS
(the ifdef change is for consistency, the spaces are slightly more important)
Attachment #91857 - Flags: review+

Comment 21

16 years ago
Comment on attachment 91857 [details] [diff] [review]
mv tabs /dev/null

a=asa (on behalf of drivers) for checkin to 1.1
Attachment #91857 - Flags: approval+
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
v
Status: RESOLVED → VERIFIED
Created attachment 95650 [details] [diff] [review]
GetSynthesizedType() must be able to return NULL on non-BeOS platforms.

Sorry, but the nsBookmarksService.cpp patch needs to be restructured slightly
(due to aggregation).  I'm re-opening this bug and attaching a slightly
modified fix... can someone with BeOS test to make sure that it doesn't regress
please?  Thanks!
Re-opening...
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 26

16 years ago
No regression. It works. And i can say more - it seems that it works even
without  #ifdef XP_BEOS part now. Though, i tested this patch with original 1.1b
release source, so i will be very grateful if you point me to another patches
since 1.1b milestone in nsBookmarkService.cpp - it those exist.

Now i prefer to let this #ifdef XP_BEOS to stay in code - for safety reason.
Sergei: Thanks for testing!  If it works without the #ifdef XP_BEOS section,
that section should be removed as the patch enables internet search & bookmark
find queries which are saved into bookmarks to work.  :)  Agreed?
(Assignee)

Comment 28

16 years ago
Sorry Robert. Little confusing happened. It seems that patch was applied
incorectly in my old sources.
When i apllied it manually, i got crash when attempting to reach Imported
Bookmarks in Bookmark manager:
loading symbols
segment violation occurred
nsBookmarksService::GetSynthesizedType(nsIRDFResource *, nsIRDFNode **):
GetSynthesizedType__18nsBookmarksServiceP14nsIRDFResourcePP10nsIRDFNode:
+00b8  ece26df8:   *          0a8b    movl    (%edx), %ecx
mozilla-bin:sc
   frame         retaddr
fcffaf08   ece2741a  nsBookmarksService::GetTarget(nsIRDFResource *,
nsIRDFResource *, int, nsIRDFNode **) + 000001da
fcffafd4   ed89e3a4  CompositeDataSourceImpl::GetTarget(nsIRDFResource *,
nsIRDFResource *, int, nsIRDFNode **) + 000000b4
fcffb004   edc07361  nsTemplateRule::ComputeAssignmentFor(nsConflictSet &,
nsTemplateMatch *, int, Value *) const + 000000d1
fcffb064   edc064e0  nsTemplateMatch::GetAssignmentFor(nsConflictSet &, int,
Value *) + 00000040
fcffb08c   edc1ce32 
nsXULTemplateBuilder::SubstituteTextReplaceVariable(nsXULTemplateBuilder *,
nsAString const &, void *) + 00000172
fcffb16c   edc1c817 
ParseAttribute__20nsXULTemplateBuilderRC9nsAStringPFP20nsXULTemplateBuilderRC9nsAStringPv_vT2Pv
+ 00000547 

So we should wait until someone (timeless, arougthopher or cls) can confirm or
reject it on up-to-date source tree for trunk. I hardly can update and build
BeOS Mozilla before Monday. But if i do it faster - i definitely check this problem
(Assignee)

Comment 29

16 years ago
my friend with 4-day old sources also got
Hunk #1 FAILED at 3077.
1 out of 1 hunk FAILED -- saving rejects to file nsBookmarksService.cpp.rej: 
***************
*** 3077,3103 ****
      // if we didn't match anything in the graph, synthesize its type
      // (which is either a bookmark or a bookmark folder, since everything
      // else is annotated)
      PRBool isContainer = PR_FALSE;
      (void)gRDFC->IsSeq(mInner, aNode, &isContainer);

      if (isContainer)
      {
        *aType =  kNC_Folder;
      }
-     else
      {
  #ifdef XP_BEOS
        //solution for BeOS - bookmarks are stored as file attributes.
-       PRBool isBookmarkedFlag = PR_FALSE;
-       rv = IsBookmarkedInternal(aNode, &isBookmarkedFlag);
-       if (!isBookmarkedFlag || NS_FAILED(rv) || (rv == NS_RDF_NO_VALUE))
-         *aType = kNC_URL;
-       else
- #endif
-        *aType = kNC_Bookmark;
      }
-     NS_ADDREF(*aType);
    }
    return(NS_OK);
  }

--- 3077,3104 ----
      // if we didn't match anything in the graph, synthesize its type
      // (which is either a bookmark or a bookmark folder, since everything
      // else is annotated)
      PRBool isContainer = PR_FALSE;
+     PRBool isBookmarkedFlag = PR_FALSE;
      (void)gRDFC->IsSeq(mInner, aNode, &isContainer);

      if (isContainer)
      {
        *aType =  kNC_Folder;
      }
+     else if (NS_SUCCEEDED(rv = IsBookmarkedInternal(aNode,
+       &isBookmarkedFlag)) && (isBookmarkedFlag == PR_TRUE))
      {
+        *aType = kNC_Bookmark;
+     }
  #ifdef XP_BEOS
+     else
+     {
        //solution for BeOS - bookmarks are stored as file attributes.
+       *aType = kNC_URL;
      }
+ #endif
+     NS_IF_ADDREF(*aType);
    }
    return(NS_OK);
  }
So question is:
is it trunk or branch patch?
(Assignee)

Comment 30

16 years ago
Now it looks like this in CVS. someone wiped all if-else conditions together
with BeOS case:
nsresult
nsBookmarksService::GetSynthesizedType(nsIRDFResource *aNode, nsIRDFNode **aType)
{
  *aType = nsnull;
  nsresult rv = mInner->GetTarget(aNode, kRDF_type, PR_TRUE, aType);
  if (NS_FAILED(rv) || (rv == NS_RDF_NO_VALUE))
  {
    // if we didn't match anything in the graph, synthesize its type
    // (which is either a bookmark or a bookmark folder, since everything
    // else is annotated)
    PRBool isContainer = PR_FALSE;
    (void)gRDFC->IsSeq(mInner, aNode, &isContainer);

    *aType = isContainer ? kNC_Folder : kNC_Bookmark;
      NS_ADDREF(*aType);
  }
  return(NS_OK);
} 
Try this  (with/without the XP_BEOS):


nsresult
nsBookmarksService::GetSynthesizedType(nsIRDFResource *aNode, nsIRDFNode **aType)
{
  *aType = nsnull;
  nsresult rv = mInner->GetTarget(aNode, kRDF_type, PR_TRUE, aType);
  if (NS_FAILED(rv) || (rv == NS_RDF_NO_VALUE))
  {
    // if we didn't match anything in the graph, synthesize its type
    // (which is either a bookmark or a bookmark folder, since everything
    // else is annotated)
    PRBool isContainer = PR_FALSE;
    PRBool isBookmarkedFlag = PR_FALSE;
    (void)gRDFC->IsSeq(mInner, aNode, &isContainer);

    if (isContainer)
    {
      *aType =  kNC_Folder;
    }
    else if (NS_SUCCEEDED(rv = IsBookmarkedInternal(aNode,
      &isBookmarkedFlag)) && (isBookmarkedFlag == PR_TRUE))
    {
       *aType = kNC_Bookmark;	
    }
#ifdef XP_BEOS
    else
    {
      //solution for BeOS - bookmarks are stored as file attributes. 
      *aType = kNC_URL;
    }
#endif
    NS_IF_ADDREF(*aType);
  }
  return(NS_OK);
}

(Assignee)

Comment 32

16 years ago
Hello Robert.
I succeed with update and build sooner than expected. Here is my test results.
I put in proposed code debug statements:
--------------------------
    else if (NS_SUCCEEDED(rv = IsBookmarkedInternal(aNode,
      &isBookmarkedFlag)) && (isBookmarkedFlag == PR_TRUE))
    {
fprintf(stdout,"kNC_Bookmark\n");
       *aType = kNC_Bookmark;	
    }
#ifdef XP_BEOS
    else
    {
      //solution for BeOS - bookmarks are stored as file attributes. 
fprintf(stdout,"BeOS - kNC_URL\n");      
      *aType = kNC_URL;
    }
#endif
fflush(stdout);
    NS_IF_ADDREF(*aType);
-----------------------------
And always when i reach Imported NetPositive bookmarks, i get in stdout:

BeOS - kNC_URL 

It means, that second condition fails for BeOS and if we ecxlude XP_BEOS case,
we'll get: *aType = nsnull; and, as result, NS_IF_ADDREF(null);
For some reason it worked, but seems wrong and i got crash on exit with some
hashtable, when excluded XP_BEOS case.
So we should save this as fallback option, though, it seems not preventing
"internet search & bookmark find queries which are saved into bookmarks to
work." - as Mozilla saves those queries in its own format, so it'll be under
first or second conditions.
OK, sounds fine to me.  Let's get reviews of the code (with #ifdef XP_BEOS) in
comment 31 so the fix can be checked in.
BTW:  "NS_IF_ADDREF(null);"  is fine, as it addrefs only if the in-param is
non-null.
(Assignee)

Comment 35

16 years ago
if there will be appliable patch, i can review it from BeOS POV.
Sergei:  the last attached patch to this bug is applicable to the tip.
(Assignee)

Comment 37

16 years ago
Comment on attachment 95650 [details] [diff] [review]
GetSynthesizedType() must be able to return NULL on non-BeOS platforms.

ok
r= sergei_d@fi.tartu.ee
Attachment #95650 - Flags: review+
Still need an sr= on this one.

Comment 39

16 years ago
Comment on attachment 95650 [details] [diff] [review]
GetSynthesizedType() must be able to return NULL on non-BeOS platforms.

sr=darin
Attachment #95650 - Flags: superreview+
Thank, Darin and Sergei.  Fix checked in.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 41

15 years ago
Broken again
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 42

15 years ago
probably due to my latest changes. :-/  sorry.
(Assignee)

Comment 43

15 years ago
nsFileSystemDataSource.cpp
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsFileSystemDataSource.cpp&root=/cvsroot&subdir=mozilla/rdf/datasource/src&command=DIFF_FRAMESET&rev1=1.119&rev2=1.120

nsXPIDLCString nativeURI;  declared but never and nowhere assigned for XP_BEOS
Component: Bookmarks → RDF
(Assignee)

Comment 44

15 years ago
tested with last cls fixes. No help. Because we still use nativeURI without
assignment of any real value
(Assignee)

Comment 45

15 years ago
Created attachment 118068 [details] [diff] [review]
patch - restores N+ bookmark import

fixed initialisation of BFile object and tests for NetPositive bookmarks path
(Assignee)

Comment 46

15 years ago
Created attachment 118069 [details] [diff] [review]
patch - added get()

same as previous - added get() to obtain char * from string.
Attachment #118068 - Attachment is obsolete: true
(Assignee)

Comment 47

15 years ago
Comment on attachment 118069 [details] [diff] [review]
patch - added get()

review request
Attachment #118069 - Flags: review?(dougt)

Updated

15 years ago
Attachment #118069 - Flags: review?(dougt) → review+
(Assignee)

Comment 48

15 years ago
fixed
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.