Closed Bug 1118977 Opened 9 years ago Closed 9 years ago

Define a constant representing an unknown folder ID

Categories

(Firefox for Android Graveyard :: Data Providers, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: rnewman, Assigned: Skandan, Mentored)

References

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(1 file, 2 obsolete files)

We use -1 in some places to indicate that no folder was found for a particular GUID. We should formalize this with a commented constant.

See the getFolderIdFromGuid method in LocalBrowserDB.
I'd like to take this on. Looking through the code I see two places where -1 is used to represent no folder found. Am I on the right track?
Hi, Prathik - Thanks for your interest! I'm going to use the "need more information" box here to alert this bug's mentors to your question. If you've got more questions, don't hesitate to do the same.
Had a bit of time off today. So uploaded a patch without being assigned, hope that's all right? :) Please tell me if there is anything I should change or do in a different way.
Attachment #8546020 - Flags: feedback?(rnewman)
Thank You Mike!
Comment on attachment 8546020 [details] [diff] [review]
Bug-1118977-fix  (Constant defined for representing Unknown Folder ID)

Review of attachment 8546020 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good first step, but you also need to find the code that *returns* -1, and make sure that it uses the constant, too. That probably means you need to move the location of the constant definition. And be aware that I'm about to touch a lot of this code as part of Bug 1077590, so I might bitrot your work!

::: mobile/android/base/db/LocalBrowserDB.java
@@ +67,5 @@
>      private static final String LOGTAG = "GeckoLocalBrowserDB";
>  
>      // Sentinel value used to indicate a failure to locate an ID for a default favicon.
>      private static final int FAVICON_ID_NOT_FOUND = Integer.MIN_VALUE;
> +    //Constant used to indicate that no particular Folder was found for particular GUID

Nit: newline before, and spacing, capitalization, punctuation, just like the comment above:

// Constant used to indicate that no folder was found for a particular GUID.
Attachment #8546020 - Flags: feedback?(rnewman) → feedback+
(In reply to Prathik Sai  [:Skandan] from comment #3)
> So uploaded a patch without being assigned,
> hope that's all right? :)

Indeed, it's encouraged. The best way to 'assign' yourself to a bug is to upload a patch for it!
Assignee: nobody → prathikcoding167
Status: NEW → ASSIGNED
Depends on: 1077590
Thanks for the guidance Richard! Let me know if anything else is not up to standard.
Attachment #8546020 - Attachment is obsolete: true
Attachment #8546211 - Flags: feedback?(rnewman)
Comment on attachment 8546211 [details] [diff] [review]
Bug-1118977-fix.diff (Changes Added)

Review of attachment 8546211 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/LocalBrowserDB.java
@@ +69,5 @@
>      // Sentinel value used to indicate a failure to locate an ID for a default favicon.
>      private static final int FAVICON_ID_NOT_FOUND = Integer.MIN_VALUE;
> +    
> +    //Constant used to indicate that no particular Folder was found for particular GUID.
> +    private static final long FOLDER_NOT_FOUND = -1L;

See my nit in Comment 5.

@@ +846,5 @@
>                                    null);
>          try {
>              final int col = c.getColumnIndexOrThrow(Bookmarks._ID);
>              if (!c.moveToFirst() || c.isNull(col)) {
> +                //Constant is used to indicate that no particular Folder was found for particular GUID.

No need for this comment.
Attachment #8546211 - Flags: feedback?(rnewman) → feedback+
Sorry about the white space nits!
Attachment #8546211 - Attachment is obsolete: true
Attachment #8547892 - Flags: feedback?(rnewman)
Comment on attachment 8547892 [details] [diff] [review]
Bug-1118977-fix.diff

Review of attachment 8547892 [details] [diff] [review]:
-----------------------------------------------------------------

You mean like the one in this patch? :D

This looks fine. "Folder" doesn't need to be capitalized mid-sentence. I'll clean this up for landing.
Attachment #8547892 - Flags: feedback?(rnewman) → review+
Flags: needinfo?(rnewman)
OK, Prathik, it's landed. Thanks for the contribution!

Let us know in IRC when you're ready for another mobile bug, or just pick one out of the queue -- I see you're active in a couple other bugs already.
Flags: needinfo?(rnewman)
Thank You for the opportunity Richard! And I've completed work on most of the other bugs I've taken. I'm learning a lot, so once I've cleared up the other bugs I've taken, I'll be sure to visit the IRC mobile channel A.S.A.P. :) Thanks Again.
https://hg.mozilla.org/mozilla-central/rev/374ab6122bf1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Target Milestone: Firefox 37 → Firefox 38
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: