All users were logged out of Bugzilla on October 13th, 2018

Define a constant representing an unknown folder ID

RESOLVED FIXED in Firefox 38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rnewman, Assigned: Skandan, Mentored)

Tracking

Trunk
Firefox 38
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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?

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
Created attachment 8546020 [details] [diff] [review]
Bug-1118977-fix  (Constant defined for representing Unknown Folder ID)

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)
(Assignee)

Comment 4

4 years ago
Thank You Mike!
(Reporter)

Comment 5

4 years ago
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+
(Reporter)

Comment 6

4 years ago
(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
(Reporter)

Updated

4 years ago
Depends on: 1077590
(Assignee)

Comment 7

4 years ago
Created attachment 8546211 [details] [diff] [review]
Bug-1118977-fix.diff (Changes Added)

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)
(Reporter)

Comment 8

4 years ago
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+
(Assignee)

Comment 9

4 years ago
Created attachment 8547892 [details] [diff] [review]
Bug-1118977-fix.diff

Sorry about the white space nits!
Attachment #8546211 - Attachment is obsolete: true
Attachment #8547892 - Flags: feedback?(rnewman)
(Reporter)

Comment 10

4 years ago
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+
(Reporter)

Updated

4 years ago
Flags: needinfo?(rnewman)
(Reporter)

Comment 12

4 years ago
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)
(Assignee)

Comment 13

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37

Updated

4 years ago
Target Milestone: Firefox 37 → Firefox 38
You need to log in before you can comment on or make changes to this bug.