1232439 - Full-page edit bookmark dialog

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[:Margaret Leibovic]

antlam, can you attach a mock here?

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_bookmarks_bookmarkmgmt_L2_4.png

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: fennec-edit-bookmark-native.png

The current "edit bookmark" dialog also supports "keywords", so we'll need that in the new dialog too.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Anthony's mockup has a "remove" icon in the actionbar, which is handy. If we are going to use the actionbar, maybe we could add "add to desktop" there too?

In the future, we could add support for saving offline.

Comment 4

[Richard Newman [:rnewman]]

Desktop lets you add tags (which is a structured list of tags), not keywords.

There is a 'keyword', which is used for search engine dispatch, but it can only be accessed in the Show All Bookmarks UI.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Created attachment 8698451 [details]
> fennec-edit-bookmark-native.png
> 
> The current "edit bookmark" dialog also supports "keywords", so we'll need
> that in the new dialog too.

I wonder how much that's actually used? I just want to avoid making these bookmarks feel overly complicated where possible. But for the time being, yes, we could just add that right at the bottom of this list.

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Anthony's mockup has a "remove" icon in the actionbar, which is handy. If we
> are going to use the actionbar, maybe we could add "add to desktop" there
> too?
> 
> In the future, we could add support for saving offline.

I don't think that's the best spot to add more actions. But moving to a full page dialog does give us more ability to add actions in the future :)

(In reply to Richard Newman [:rnewman] from comment #4)
> Desktop lets you add tags (which is a structured list of tags), not keywords.
> 
> There is a 'keyword', which is used for search engine dispatch, but it can
> only be accessed in the Show All Bookmarks UI.

Can we unify here somehow? If they use "tags" maybe we should call ours "tags" as well? unless there's a reason not to?

Comment 6

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #5)

> (In reply to Richard Newman [:rnewman] from comment #4)
> > Desktop lets you add tags (which is a structured list of tags), not keywords.
> > 
> > There is a 'keyword', which is used for search engine dispatch, but it can
> > only be accessed in the Show All Bookmarks UI.
> 
> Can we unify here somehow? If they use "tags" maybe we should call ours
> "tags" as well? unless there's a reason not to?

"tags" and "keywords" are two different things, both of which are supported on desktop. Right now on mobile we only support keywords.

Confusing, I know.

I think we should maintain the status quo for now, and continue to support only keywords on mobile. Or maybe we should hide this field, and only show it if you happen to have synced a bookmark with a keyword from desktop.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #6)
> "tags" and "keywords" are two different things, both of which are supported
> on desktop. Right now on mobile we only support keywords.
> 
> Confusing, I know.
> 
> I think we should maintain the status quo for now, and continue to support
> only keywords on mobile. Or maybe we should hide this field, and only show
> it if you happen to have synced a bookmark with a keyword from desktop.

I'm OK with supporting less! 

Either way, this added option should just be added to the bottom of this page I think.

Comment 8

[Andrzej Hunt :ahunt]

I'm currently working on porting the existing dialog functionality (and can look at the keywords/tags/folders as a later step).

I've been looking at the Android style guide ( https://www.google.com/design/spec/components/dialogs.html#dialogs-full-screen-dialogs ), and they suggest using an X instead of back-arrow (which would effectively replace the "cancel" button on the existing dialog). Do we want to do that here for consistency? I.e. we'd have "X" to cancel in the top-left, "Save" in the top right. Doing that wouldn't leave much space for the bin (delete) and/or "Add to desktop" though.

Comment 9

[lgbmontenegroii]

Glad to see that this issue is receiving the much needed attention. I suggest taking a look at the UI of the of the app KeePass2Android. X to cancel is a good choice to minimize unnecessary UI options. Adding the 3 dotted menu with additional options upon pressing (delete, add keywords, save) seems good or placing a button on the lower right that expands when selected and offers additional actions is also viable.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Andrzej Hunt :ahunt from comment #8)
> I'm currently working on porting the existing dialog functionality (and can
> look at the keywords/tags/folders as a later step).
> 
> I've been looking at the Android style guide (
> https://www.google.com/design/spec/components/dialogs.html#dialogs-full-
> screen-dialogs ), and they suggest using an X instead of back-arrow (which
> would effectively replace the "cancel" button on the existing dialog). Do we
> want to do that here for consistency? I.e. we'd have "X" to cancel in the
> top-left, "Save" in the top right. Doing that wouldn't leave much space for
> the bin (delete) and/or "Add to desktop" though.

I'd like to keep with the mental model of going "down a level" first since seeing an 'x' in the top left and a 'trash' in the top left is really confusing.

The Back arrow would save as well so I think that's OK - let's try this first :)

Comment 11

[Andrzej Hunt :ahunt]

(In reply to Anthony Lam (:antlam) from comment #10)
> I'd like to keep with the mental model of going "down a level" first since
> seeing an 'x' in the top left and a 'trash' in the top left is really
> confusing.
> 
> The Back arrow would save as well so I think that's OK - let's try this
> first :)

One remaining issue: what do we do if the dialog content is unusable? In the current dialog we disable the save button if either (1) the bookmark title is empty or (2) the keyword is mangled (i.e. has leading or trailing spaces).

Would it be useful to show a dialog containing something like this in those cases?

"Please insert a bookmark title" (or "The keyword cannot contain leading or trailing spaces")
|Return to bookmark| |Discard Changes|

We can also show an Error icon/text as soon as the user removes the bookmark title / enters an invalid keyword (examples visible at https://www.google.com/search?q=TextView+setError&source=lnms&tbm=isch ), it might be nicer to use that and just show the following instead?

"Bookmark changes can't be saved"
|Return to bookmark| |Discard Changes|

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Andrzej Hunt :ahunt from comment #11)
> One remaining issue: what do we do if the dialog content is unusable? In the
> current dialog we disable the save button if either (1) the bookmark title
> is empty or (2) the keyword is mangled (i.e. has leading or trailing spaces).
> 
> Would it be useful to show a dialog containing something like this in those
> cases?
> 
> "Please insert a bookmark title" (or "The keyword cannot contain leading or
> trailing spaces")
> |Return to bookmark| |Discard Changes|
> 
> We can also show an Error icon/text as soon as the user removes the bookmark
> title / enters an invalid keyword (examples visible at
> https://www.google.com/search?q=TextView+setError&source=lnms&tbm=isch ), it
> might be nicer to use that and just show the following instead?
> 
> "Bookmark changes can't be saved"
> |Return to bookmark| |Discard Changes|

If there's an error with any of the fields, we should show that validation and feedback in the form UI rather than a dialog.

I think just highlighting the underline red and adding feedback underneath as in the guidelines [1] would be best.

[1] https://www.google.com/design/spec/components/text-fields.html#text-fields-single-line-text-field

Comment 13

[Andrzej Hunt :ahunt]

(In reply to Anthony Lam (:antlam) from comment #12)
> (In reply to Andrzej Hunt :ahunt from comment #11)
> > One remaining issue: what do we do if the dialog content is unusable? In the
> > current dialog we disable the save button if either (1) the bookmark title
> > is empty or (2) the keyword is mangled (i.e. has leading or trailing spaces).
> > 
> > Would it be useful to show a dialog containing something like this in those
> > cases?
> > 
> > "Please insert a bookmark title" (or "The keyword cannot contain leading or
> > trailing spaces")
> > |Return to bookmark| |Discard Changes|
> > 
> > We can also show an Error icon/text as soon as the user removes the bookmark
> > title / enters an invalid keyword (examples visible at
> > https://www.google.com/search?q=TextView+setError&source=lnms&tbm=isch ), it
> > might be nicer to use that and just show the following instead?
> > 
> > "Bookmark changes can't be saved"
> > |Return to bookmark| |Discard Changes|
> 
> If there's an error with any of the fields, we should show that validation
> and feedback in the form UI rather than a dialog.
> 
> I think just highlighting the underline red and adding feedback underneath
> as in the guidelines [1] would be best.
> 
> [1]
> https://www.google.com/design/spec/components/text-fields.html#text-fields-
> single-line-text-field

To get these error messages we'll need to replace FloatingHintEditText (our homegrown implementation) with TextInputLayout from the Design Support Library, tracked in Bug 1227325.

(The relevant error setting method is in http://developer.android.com/reference/android/support/design/widget/TextInputLayout.html#setError%28java.lang.CharSequence%29 )

Comment 14

[Richard Newman [:rnewman]]

(In reply to Anthony Lam (:antlam) from comment #12)

> If there's an error with any of the fields, we should show that validation
> and feedback in the form UI rather than a dialog.

Even better, trim leading and trailing whitespace from the keyword automatically, and disable saving with a whitespace-only title.

Comment 15

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowerDB r=mcomella

Review commit: https://reviewboard.mozilla.org/r/30829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30829/

Comment 16

[Andrzej Hunt :ahunt]

(In reply to Richard Newman [:rnewman] from comment #14)
> (In reply to Anthony Lam (:antlam) from comment #12)
> 
> > If there's an error with any of the fields, we should show that validation
> > and feedback in the form UI rather than a dialog.
> 
> Even better, trim leading and trailing whitespace from the keyword
> automatically, and disable saving with a whitespace-only title.

Looking at the the existing implementation: we already trim whitespace from the keyword (however sincew we disable the save button when that happens, so the trimming isn't actually of any use there) - so all that's needed is to remove the validation.

I think it might be better to trim whitespace from the title when saving too - in this case, if the user enters a purely whitespace title, we can just act as if there's no title at all (which the UI in the rest of the app seems to handle without issues), rather than having to specially handle this case?

Comment 17

[Andrzej Hunt :ahunt]

Attachment: WIP draft patch screenshot

(In reply to Andrzej Hunt :ahunt from comment #15)
> Created attachment 8707671 [details]
> MozReview Request: Bug 1232439 - WIP: Convert edit bookmark dialog to full
> screen fragment
> 
> Review commit: https://reviewboard.mozilla.org/r/30829/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/30829/

This is still a draft patch - there are still a few things I need to clean up (hopefully I can break it down into smaller comitts). I've also attached a screenshot of the current appearance. The icons need fixing, I'm also unhappy with the spacing between the text fields, I have a feeling that might be hard to fix because of how TextInputLayout deals with padding for the floating errors/hints.

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8707671 [details]
MozReview Request: Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowerDB r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30829/diff/1-2/

Comment 19

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment r=mcomella

Review commit: https://reviewboard.mozilla.org/r/34211/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34211/

Comment 20

[Andrzej Hunt :ahunt]

Comment on attachment 8717560 [details]
MozReview Request: Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment r=mcomella

Cancelling review for now - I've just realised there were some more changes / simplifications I'd forgotten to do.

Comment 21

[Andrzej Hunt :ahunt]

Comment on attachment 8707671 [details]
MozReview Request: Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowerDB r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30829/diff/2-3/

Comment 22

[Andrzej Hunt :ahunt]

Comment on attachment 8717560 [details]
MozReview Request: Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34211/diff/1-2/

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8707671 [details]
MozReview Request: Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowerDB r=mcomella

https://reviewboard.mozilla.org/r/30829/#review32531

I'm not sure what the parent uri is but Margaret & liuche tell me it's for bookmark folders in which case bumping the modified time makes sense.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1035
(Diff revision 3)
> +    public void removeBookmarkWithID(ContentResolver cr, int id) {

nit: `final` args

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1038
(Diff revision 3)
> +        bumpParents(cr, Bookmarks._ID, String.valueOf(id));

The other invocation has the comment: `// Do this now so that the items still exist!` We should also add this here.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8717560 [details]
MozReview Request: Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment r=mcomella

https://reviewboard.mozilla.org/r/34211/#review32533

Going back via the back button does not display the "Bookmark updated" toast while hit the top-level back does – is this intentional?

Also, this UI is confusing – how do I go back without saving my changes? How do I go back & save my changes? I feel like there should be a "save"/"cancel" button set here.

I'll finish the rest of the review once ^ is resolved.

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:43
(Diff revision 2)
> +    private static final String SAVED_STATE_URL= "url";

nit: `... = ...`

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:62
(Diff revision 2)
> +            mBookmark = new Bookmark(savedInstanceState.getInt(SAVED_STATE_ID),

I think it'd be slicker if you made this (and `onSaveInstanceState` a function of the `Bookmark` class).

e.g. `Bookmark.newFromBundle` and `Bookmark.saveToBundle`.

Then the constants can also be members of the `Bookmark` class rather than the dialog.

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:72
(Diff revision 2)
>      private class Bookmark {

nit: `private static class`

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:94
(Diff revision 2)
> -    private class EditBookmarkTextWatcher implements TextWatcher {
> +    private class LocationTextWatcher implements TextWatcher {

nit: What "Location" represented was unclear to me – perhaps "BookmarkUri"?

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:106
(Diff revision 2)
> -            // Disable if the we're disabled or the paired partner is disabled
> +            boolean enabled = (s.toString().trim().length() > 0);

nit: It's unclear to me what `enabled` is – perhaps it should be `isFieldEmpty`? The code can speak for what should happen when the field is empty.

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:127
(Diff revision 2)
> -    /**
> +    @Nullable

I had no idea that this works for return values – nice!

::: mobile/android/base/java/org/mozilla/gecko/EditBookmarkDialog.java:269
(Diff revision 2)
> +        transaction.add(android.R.id.content, this).addToBackStack(null).commitAllowingStateLoss();

We're drawing over `BrowserApp`, causing some overdraw inefficencies. It's not terrible, since there isn't too much to interact here, but let's eliminate it if we can.

::: mobile/android/base/resources/layout/bookmark_edit.xml:11
(Diff revision 2)
> -              android:layout_height="match_parent">
> +              android:layout_height="match_parent"

I think full page dialogs look a bit silly on tablet. Did you consider an alternative layout for tablet?

That being said, I presume it's not used often so it's probably not worth the time.

Also, if we don't plan to recompose this, we could make this an Activity, rather than a Fragment, which simplifies things (e.g. there's an overdraw issue here).

::: mobile/android/base/resources/layout/bookmark_edit.xml:13
(Diff revision 2)
> +              android:theme="@style/GeckoAlertDialog">

I was suprised to see `android:theme` (as opposed to `style`) here, but as it turns out android:theme is an appCompat thing: http://androidxref.com/6.0.0_r1/xref/frameworks/support/v7/appcompat/res/values/attrs.xml#505

This could be simpler if this was an `Activity` though (see my thoughts below).

::: mobile/android/base/resources/layout/bookmark_edit.xml:16
(Diff revision 2)
> +            android:id="@+id/my_toolbar"

nit: -> `+id/toolbar`

::: mobile/android/base/resources/layout/bookmark_edit.xml:19
(Diff revision 2)
> +            android:layout_alignParentTop="true"

This is unnecessary – this is a `RelativeLayout` param but your parent is a `LinearLayout`.

::: mobile/android/base/resources/layout/bookmark_edit.xml:28
(Diff revision 2)
> +            android:id="@+id/scrollView"

nit: -> `scrollview` or `scroll_view`

::: mobile/android/base/resources/layout/bookmark_edit.xml:45
(Diff revision 2)
> -                android:singleLine="true"
> +                        android:singleLine="true"

Since we have a full screen, should we still limit each input to `singleLine`?

::: mobile/android/base/resources/layout/bookmark_edit.xml:47
(Diff revision 2)
> -                />
> +                    <requestFocus/>

I think you meant this as an attr?

::: mobile/android/base/resources/menu/edit_bookmarks_menu.xml:6
(Diff revision 2)
> +<menu xmlns:android="http://schemas.android.com/apk/res/android" xmlns:app="http://schemas.android.com/apk/res-auto">
> +
> +    <item android:id="@+id/bin" android:icon="@android:drawable/ic_menu_delete" android:title="@string/bookmark_remove" app:showAsAction="always"/>

nit: Our style is that we have one xml attr one each line.

Comment 25

[Andrzej Hunt :ahunt]

(In reply to Michael Comella (:mcomella) from comment #24)
> Also, if we don't plan to recompose this, we could make this an Activity,
> rather than a Fragment, which simplifies things (e.g. there's an overdraw
> issue here).
> 

I've investigated with turning this into an activity: one issue is that we're creating snackbars when shutting down the "dialog", they attach to the current activity, and get killed when activity dies - which means that we can't actually show these snackbars. We'd have to pass in the parent activity to attach the Snackbar there, or move our snackbar code into the calling activity (or provide some callback mechanism for this - which seems cleanest).

However we _might_ want to reuse this dialog for creating/editing new bookmark folders (we have many of the same fields, i.e. name and bookmark folder), in this case keeping it as a fragment seems to simplify the flow. I.e. we'll need to support the following:
1. Edit bookmark -> opens edit bookmark "dialog"
2. Modify folder -> opens folder selector "dialog" (also being implemented as a fragment, that could change)
3. Create a new folder from the folder selector -> opens new folder dialog (i.e. the edit bookmark dialog with only 2 fields being shown)
(4. Modify parent folder of this new folder -> open another folder selector.)

That however doesn't stop us from creating a wrapper Activity, which then loads the EditBookmark fragment by default, and we can add the callbacks so that the parent activity can show snackbars when the bookmark is edited (but not show snackbars when we create the folder)?

Comment 26

[Andrzej Hunt :ahunt]

I'm not actively looking into this anymore. I do however have some WIP patches that will be uploaded to Bug 972193. If anyone is interested in taking this up again I'd reccomend looking there, especially since whatever architecture we choose for a full-screen dialog impacts how bookmark folder selection can be implemented.

(I.e. it seems to be useful to have one activity deal with all bookmark related tasks, but we have multiple UIs that need to be swapped in/out for actual bookmark editing, followed by folder selection, and if needed folder creation/editing.)

Comment 27

[Tom Klein]

Attachment: Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowserDB

Rebased by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/98106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98106/

Comment 28

[Tom Klein]

Attachment: Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment

Rebased by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/98108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98108/

Comment 29

[Tom Klein]

Attachment: [WIP] Bug 1232439 (972193) - Part 1: introduce BrowserDB.getBookmarkFolders

Rebased by Tom Klein.

Review commit: https://reviewboard.mozilla.org/r/98110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98110/

Comment 30

[Tom Klein]

Attachment: [WIP] Bug 1232439 (972193) - Add more database folder methods for bookmark edit dialogs.

Review commit: https://reviewboard.mozilla.org/r/98112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98112/

Comment 31

[Tom Klein]

Attachment: [WIP] Bug 1232439 (972193) - Add FolderTreeView for choosing a folder from a list.

Review commit: https://reviewboard.mozilla.org/r/98114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98114/

Comment 32

[Tom Klein]

Attachment: [WIP] Bug 1232439 (972193) - Create base classes for use in bookmark edit dialogs.

BookmarkToolbarDialog creates just the toolbar; its subclass
BookmarkFolderTreeDialog additionally includes a FolderTreeView for displaying a
list of folders.

Review commit: https://reviewboard.mozilla.org/r/98116/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98116/

Comment 33

[Tom Klein]

Attachment: [WIP] Bug 1232439 (972193) - Add "Choose Folder" bookmark edit dialog option.

Review commit: https://reviewboard.mozilla.org/r/98118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98118/

Comment 34

[Tom Klein]

Attachment: [WIP] Bug 1232439 (972193) - Add "Create folder" bookmark edit dialog option.

Review commit: https://reviewboard.mozilla.org/r/98120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98120/

Comment 35

[Tom Klein]

Attachment: [WIP] Bug 1232439 (972193) - Add dialog for selecting folder in which to place new bookmarks folder.

Review commit: https://reviewboard.mozilla.org/r/98122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98122/

Comment 36

[Tom Klein]

This isn't ready for review yet, but I think it's ready for feedback.  Could you take a look when you can Andrzej?  The version here implements this bug and bug 972193.  The commits are oriented toward fixing both bugs at once, but if the approach looks good then I can pull them apart and can land them separately if that's better.

I used DialogFragments for each of the edit screens.

I think the code is in pretty good shape, in spite of the following long list of known issues and questions:

1) The main functional issue with this version is that for the text fields that jump you to a new folder selection dialog, you have to tap twice: once to focus the field, and then again to open the new dialog.  (I expect I'll have to intercept or forward a click event to fix that, unless somebody has a better way.)

2) In the "Choose folder" dialog, the "New folder" item should scroll along with the folder tree items, but doesn't yet.  (I hoped that was fixable by throwing everything in a ScrollView, setting layout_height="wrap_content" on the RecyclerView list, and then setting setNestedScrollingEnabled(false), but I haven't gotten that to work yet.  Again I'd be happy for any thoughts on that :-)

3) The icons will need work - I just grabbed what I could find.  I could use some input on that, and the rest of the UI as it stands, if/when it's ready for that.  One note: I removed the "bin" option from the top right of the "Edit bookmark" dialog and replaced it, on all panels that use that action, with a checkmark used to save the input.  Previously the back arrow (on the left of the toolbar) was used to save/confirm the "Edit bookmark" dialog, but on all the other panels the back arrow is used to cancel a particular panel, so for consistency's sake I think they should all work the same.  There's a "remove" option on the menu you get by long-pressing a bookmark, so a bookmark can be deleted there instead.  (The option doesn't currently exist in the menu you get if you star a page and then click on the "Options" action in the resulting snackbar, but it seems like it should be added there as well.)  Or maybe there's some other good way to add it to the current "Edit bookmark" dialog panel?

5) As Andrzej points out in the code and Richard has mentioned elsewhere, if there are multiple bookmarks with the same url, there's no telling which one you'll wind up editing, and the user is never notified of that fact.

6) It seems the "Desktop" folders exist (currently unexposed I guess) even if you've never synced, and with the new dialog they show up as folders you can use for bookmarks.  Is that worth worrying over?

7) It's not allowed to save a bookmark, or create a new folder, in the "Desktop" folder - is that correct?  That's the way it's implemented here.

8) I should add some tests for the new folder db capabilities.  Others?  (I haven't looked at what kind of robocop tests we currently have yet.)

And then some cleanup issues remaining:
9) All of the new strings, dimensions, etc. still need to be moved into resources.

10) I've added a number of new classes associated with the bookmarks editing dialog - should we create a new bookmarks package?  Eventually we'll want to be able to change folder names and move them etc. as well, so there will probably be more to add then.

11) Should all ids for database entries be longs in java?

12) The "Create folder" dialog doesn't have visible input validation yet (though it already checks behind the scenes).

13) As Michael pointed out, maybe we should adjust the layout on tablets?

14) The padding on the folder trees is missing in an api 16 emulator; I haven't looked into that yet.

15) There are various other more or less minor TODOs in the code.

Comment 37

[Tom Klein]

Attachment: [WIP] Bug 1232439 (972193) - Only include desktop folder if a sync has occurred.

With some minor cleanup edits in the folder tree fetching code.

Review commit: https://reviewboard.mozilla.org/r/101006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/101006/

Comment 38

[Tom Klein]

Comment on attachment 8817944 [details]
Bug 1232439 - Part 1: Implement removeBookmarkWithID in BrowserDB

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98106/diff/1-2/

Comment 39

[Tom Klein]

Comment on attachment 8817945 [details]
Bug 1232439 - Part 2: convert edit bookmark dialog into fullscreen fragment

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98108/diff/1-2/

Comment 40

[Tom Klein]

Comment on attachment 8817946 [details]
[WIP] Bug 1232439 (972193) - Part 1: introduce BrowserDB.getBookmarkFolders

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98110/diff/1-2/

Comment 41

[Tom Klein]

Comment on attachment 8817947 [details]
[WIP] Bug 1232439 (972193) - Add more database folder methods for bookmark edit dialogs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98112/diff/1-2/

Comment 42

[Tom Klein]

Comment on attachment 8817948 [details]
[WIP] Bug 1232439 (972193) - Add FolderTreeView for choosing a folder from a list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98114/diff/1-2/

Comment 43

[Tom Klein]

Comment on attachment 8817949 [details]
[WIP] Bug 1232439 (972193) - Create base classes for use in bookmark edit dialogs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98116/diff/1-2/

Comment 44

[Tom Klein]

Comment on attachment 8817950 [details]
[WIP] Bug 1232439 (972193) - Add "Choose Folder" bookmark edit dialog option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98118/diff/1-2/

Comment 45

[Tom Klein]

Comment on attachment 8817951 [details]
[WIP] Bug 1232439 (972193) - Add "Create folder" bookmark edit dialog option.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98120/diff/1-2/

Comment 46

[Tom Klein]

Comment on attachment 8817952 [details]
[WIP] Bug 1232439 (972193) - Add dialog for selecting folder in which to place new bookmarks folder.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98122/diff/1-2/

Comment 47

[Tom Klein]

Getting closer; ready for UI review I think.

Some known issues/questions:
a) Are there any restrictions on folder names?  Some quick testing seemed to indicate desktop was happy with pretty much anything.  The only restriction I have in place is that a folder name can't be empty (i.e. whitespace only).  Desktop even allows that actually, and displays it in UI as "(no title)", or as " " in the case of names that are whitespace.  Hmm, I guess maybe that means we already deal with those cases when we sync; I'll take a closer look at what we handle via sync.

b) The folder tree view doesn't currently display correctly for RTL (assuming the layout should be something like this: http://demos.telerik.com/kendo-ui/treeview/right-to-left-support).  Right now I'm planning on that being a follow up bug, maybe with some input from the folks who have been working on RTL recently.

c) The keyboard needs to get dismissed in more places than it currently is.

Comment 48

[Tom Klein]

Attachment: Edit bookmark dialogs screenshots

Anthony, could you take a look at the attached screenshots and see what you think?  You progress from 1 to 2 by tapping the "Folder" field, from 2 to 3 by tapping "New folder...", and from 3 to 4 by tapping the "Parent folder" field.

Aside from layout and icon issues (please comment on those as well as you see fit), the main issue that needs resolving is how to handle save and cancel on each dialog.  The way it currently works is:
a) The toolbar back button (at the top left) cancels the current dialog and returns to the previous one without saving whatever's been input.
b) The hardware back button does the same.
c) The green checkmark in the upper right saves the data entered in the current dialog and returns to the previous dialog (in dialogs 2 and 4 there's no input to save, so no checkmark - you either click back or you click on something in the dialog that itself causes a dialog change).

If we have the toolbar back button do a save instead of a cancel, then the only way to cancel would be to use the hardware back button; the two available back buttons would then perform different operations.

I assume we'll need new icons to replace the checkmarks at some point.

One last note: all folders past a certain depth (depending on the width available) are displayed with the same indent so that at least a minimum amount of room is provided to read the folder name - so in the screenshots, folder 9 is actually a subfolder of folder 8, and folder 10 is actually a subfolder of folder 9.

Comment 49

[Tom Klein]

Attachment: Whitespace folder name handling

(In reply to Tom Klein from comment #47)
> a) Are there any restrictions on folder names?  Some quick testing seemed to
> indicate desktop was happy with pretty much anything.  The only restriction
> I have in place is that a folder name can't be empty (i.e. whitespace only).
> Desktop even allows that actually, and displays it in UI as "(no title)", or
> as " " in the case of names that are whitespace.  Hmm, I guess maybe that
> means we already deal with those cases when we sync; I'll take a closer look
> at what we handle via sync.

We do indeed get "" and " " and other whitespace folder names via sync, so I've switched to having no restrictions on allowed folder names in the bookmark edit dialog.  The attached screenshot shows how fennec displays such folder names in bookmarks browsing on the left, and on the right how I've adjusted the edit dialog to display "(no title)" instead of "" so that it doesn't appear as though no folder has been selected at all.  I've done the same in the folder tree views in the edit dialog, to match desktop.  Desktop would additionaly show "(no title)" instead of "" in the left hand folder browsing case, if that was something we wanted to match (in a separate bug).

Comment 50

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Amazing! Thanks for the feedback Tom. This is coming along nicely!

I'm going to NI Jack and Tori here too since they've been working on some of the Bookmarks Management UX

Comment 51

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Speaking to Tori about this more, I agree, it seems like this bug is getting a bit out of scope. 

While folder management and all that is stuff we definitely want to do too, I think we should keep this bug to be about this full-page dialog UX only. 

Let's deal with the UX of the "Edit Bookmark" dialog only. The design in comment 1 still applies as a starting point, but we can ignore the "folder" item that I have in there and leave that for another bug.

Tori will work on updating the design spec for this "Edit Bookmark" dialog in the mean time to address the cases Tom brought up above :)

Comment 52

[Tom Klein]

Hi Tori,

I'm not looking for a specific date, but for my own planning purposes, can I ask if there's a timeline for the UX on this bug?  Thanks!

Comment 53

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Tom Klein from comment #52)
> Hi Tori,
> 
> I'm not looking for a specific date, but for my own planning purposes, can I
> ask if there's a timeline for the UX on this bug?  Thanks!

Hi Tom,
As the EPM driving this bookmark management enhancement, I'm expecting UX to come out spec around 3 weeks from now (considering upcoming Chinese New Year holidays). 
The coming new design should cover this editing dialog and folder management as well. 
If you will still be available and interested in taking some of the engineering tasks around that time, let's keep in sync.

Comment 54

[Tom Klein]

Thanks Wesley - I'll be happy to help out where I can once things are ready :)

Comment 55

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Tom Klein from comment #54)
> Thanks Wesley - I'll be happy to help out where I can once things are ready
> :)

https://drive.google.com/drive/folders/0B4dMhI4hp32OcF9uYU5FWHYxQWM  
We have the UX design ready for development, and :jwu is going to begin working on bookmark folder stuff.

Comment 56

[Tom Klein]

I'm available to help out. Feel free to bring up work on individual bugs where I could jump in, either via bugzilla or email.  I could start working on this bug again on 2/22 or 2/23 for example if you'd like, but I'll wait to hear from someone before starting.

Comment 57

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Tom Klein from comment #56)
> I'm available to help out. Feel free to bring up work on individual bugs
> where I could jump in, either via bugzilla or email.  I could start working
> on this bug again on 2/22 or 2/23 for example if you'd like, but I'll wait
> to hear from someone before starting.

That's great!
Feel free to go ahead and take this one. There's also a list of broken-down user story bugs[1] to pick from . 
I'm not a dev so :jwu is a more technical person if you need someone to talk to regarding specific implementation details. 


[1] https://bugzilla.mozilla.org/showdependencytree.cgi?id=1329127&hide_resolved=1

Comment 58

[Jing-wei Wu [:jwu]]

Hi Tom, I'm currently working on bookmark management and I think this one might be the first beginning of whole related bugs. So maybe I can send a patch next week and you can help review or see anything else can be improved?

Comment 59

[Tom Klein]

I'll mark my patches as obsolete.

Comment 60

[Jing-wei Wu [:jwu]]

Attachment: Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review commit: https://reviewboard.mozilla.org/r/120816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120816/

Comment 61

[Jing-wei Wu [:jwu]]

Attachment: Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review commit: https://reviewboard.mozilla.org/r/120810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120810/

Comment 62

[Jing-wei Wu [:jwu]]

Attachment: Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review commit: https://reviewboard.mozilla.org/r/120812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120812/

Comment 63

[Jing-wei Wu [:jwu]]

Attachment: Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review commit: https://reviewboard.mozilla.org/r/120814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120814/

Comment 64

[Jing-wei Wu [:jwu]]

To enable full-page dialog, "ui.bookmark.mobilefolder.enabled" in about:config needs to be turn on.

Comment 65

[Jing-wei Wu [:jwu]]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120816/diff/1-2/

Comment 66

[Jing-wei Wu [:jwu]]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120810/diff/1-2/

Comment 67

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/1-2/

Comment 68

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/1-2/

Comment 69

[:Grisha Kruglov]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

https://reviewboard.mozilla.org/r/120816/#review125146

Reviewing this will be much easier if you briefly described the reason for this refactoring.
I think I understand this though, looking at your other patches and at Bug 1342354. You're going to use the `isEnabled` method to decide if you're going to open a new edit fragment vs an existing dialog.

Bookmark management is a big functionality change, and due to Sync might indirectly affect other clients that a user has connected. You will be allowing users to make significant changes to the structure of their bookmark tree, which might trigger a number of sync-related issues.

I don't think a build flag is a good approach here. You need a remote kill-switch for this in case things go south, and you need a way to slowly roll this out. I suggest setting up a Switchboard experiment for this, and your isEnabled check should like something like this:

`if AppConstants.NIGHTLY_BUILD && SwitchBoard.isInExperiment(context, Experiments.FULL_BOOKMARK_MANAGEMENT)`.

Bug 1348820 is a good starting point - it sets up a switchboard experiment to enable activity stream for 50% of nightly users. You probably want something very similar here, perhaps experiment which is enabled for 100%, but acts as your remote kill-switch - _in addition_ to the IS_NIGHTLY check to really ensure this won't ride the trains by accident before it's ready.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkPrefs.java:10
(Diff revision 2)
> +
> +package org.mozilla.gecko.bookmark;
> +
> +import org.mozilla.gecko.PrefsHelper;
> +
> +public class BookmarkPrefs {

Please add a class comment describing what this is and how it's used.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkPrefs.java:15
(Diff revision 2)
> +public class BookmarkPrefs {
> +
> +    private static final String BOOKMARK_MOBILE_FOLDER_PREF = "ui.bookmark.mobilefolder.enabled";
> +
> +    private final PrefsHelper.PrefHandler prefObserver;
> +    private boolean mIsMobileFolderPrefOn;

Don't mix naming conventions. I don't believe we use hungarian notation much, and certainly not for any new code. Simply `isMobileFolderPrefOn` will do.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkPrefs.java:37
(Diff revision 2)
> +    }
> +
> +    private class PrefHandler extends PrefsHelper.PrefHandlerBase {
> +        @Override
> +        public void prefValue(String pref, boolean value) {
> +            if (value == mIsMobileFolderPrefOn) {

No need for this check.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:152
(Diff revision 2)
>              mCanLoadHint = DEFAULT_CAN_LOAD_HINT;
>          }
>  
>          mIsLoaded = false;
> +
> +        mBookmarkPrefs = new BookmarkPrefs();

Why is this init stuff here and not in BookmarksPanel's onCreateView, as it was before? Doesn't this mean you're now setting up a bookmark-specific observer whenever any HomeFragment instance is created? That doesn't seem right to me.

Comment 70

[:Grisha Kruglov]

Sebastian, could you please help provide some guidance on setting up a switchboard experiment on the kinto side of things? Management url in https://wiki.mozilla.org/Mobile/Fennec/Android/Downloadable_Content doesn't seem to work for me (perhaps Bug 1313038 is helpful).

Comment 71

[:Grisha Kruglov]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

https://reviewboard.mozilla.org/r/120812/#review125174

Quick first pass, I'll do another one separately. Please `histedit` your patches before resubmitting.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkCacheHelper.java:43
(Diff revision 2)
> - *     })
> - *     .duration(Snackbar.LENGTH_LONG)
> - *     .buildAndShow();
> - * </pre>
>   */
>  public class BookmarkCacheHelper {

Please roll these changes into your "Part 2" patch. Otherwise I have to review two different versions of this code :/

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkCacheHelper.java:43
(Diff revision 2)
> - *     })
> - *     .duration(Snackbar.LENGTH_LONG)
> - *     .buildAndShow();
> - * </pre>
>   */
>  public class BookmarkCacheHelper {

This doesn't feel right to me.

There are two major things going on here.

2) 1) You have conflated a simple thing, a cache, with a complicated thing - collection of functions to modify bookmarks, folders, etc in live storage. Keep your cache very simple, and move db operations to the data layer. For these data types, we have `BrowserProvider` and `LocalBrowserProvider`, which handle operations on the database - deletions, removals, etc. `BrowserProvider` handles various compound operations, and provides you with ways to perform queries transactionally.

2) You have implemented update/remove operations in a very unsafe manner. You should be wrapping these in transactions, and performing them atomically.

For a very trivial case of why this is bad, consider what will happen if you kill fennec while you're trying to delete a deeply nested folder structure.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/EditBookmarkTask.java:1
(Diff revision 2)
> +package org.mozilla.gecko.bookmark;

license

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:61
(Diff revision 2)
>   * HomeFragment is an empty fragment that can be added to the HomePager.
>   * Subclasses can add their own views.
>   * <p>
>   * The containing activity <b>must</b> implement {@link OnUrlOpenListener}.
>   */
> -public abstract class HomeFragment extends Fragment {
> +public abstract class HomeFragment extends Fragment implements BookmarkEditFragment.Callbacks {

Is there a good reason why an abstract base class used by unrelated UI (classic top sites, activity stream, etc) is implementing bookmark-related callbacks?

This doesn't seem right to me.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:310
(Diff revision 2)
>  
>              return true;
>          }
>  
>          if (itemId == R.id.home_edit_bookmark) {
> +            if (mBookmarkPrefs.isEnabled()) {

See my comments on this from Part 1.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:410
(Diff revision 2)
>  
>          load();
>          mIsLoaded = true;
>      }
>  
> -    protected static class RemoveItemByUrlTask extends UIAsyncTask.WithoutParams<Void> {
> +    class RemoveItemByUrlTask extends UIAsyncTask.WithoutParams<Void> {

Avoid this unless there's a very good reason. If you need to access something in the wrapping class, just pass it in.

Otherwise there's a good chance you're going to leak HomeFragment instance here.

::: mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java:467
(Diff revision 2)
>  
>          @Override
>          public void onPostExecute(Void result) {
> +            // Currently only support restore bookmark.
> +            if (mType == RemoveItemType.BOOKMARKS) {
> +                SnackbarBuilder.builder((Activity) mContext)

This is duplicated almost verbatim in EditBookmarkTask. What's going on here?

::: mobile/android/base/locales/en-US/android_strings.dtd:91
(Diff revision 2)
>  <!-- Localization note (bookmark_folder_items): The variable is replaced by the number of items
>       in the folder. -->
>  <!ENTITY bookmark_folder_items "&formatD; items">
>  <!ENTITY bookmark_folder_one_item "1 item">
> +<!ENTITY bookmark_parent_folder "Parent Folder">
> +<!ENTITY bookmark_undo "UNDO">

Why all caps? No other string is in all caps.

Comment 72

[:Grisha Kruglov]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/120810/#review125178

Please see my comments on your other patch, which changes a lot of this code. As you find yourself refactoring your work, it's a very good idea to go through your patch series and move things around. Changes in Part 3 _should_ be part of this patch. This kind of re-shuffling should be pretty easy via `hg histedit`.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkCacheHelper.java:52
(Diff revision 2)
> + *     })
> + *     .duration(Snackbar.LENGTH_LONG)
> + *     .buildAndShow();
> + * </pre>
> + */
> +public class BookmarkCacheHelper {

I don't think this class should exist in its current form. Please see my comment regarding refactoring this, and moving actual editing logic into `BrowserProvider` and friends.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1205
(Diff revision 2)
>          addBookmarkItem(cr, title, uri, folderId);
>          return true;
>      }
>  
> +    @Override
> +    public boolean restoreBookmark(ContentResolver cr, String title, String url, int type,

This isn't going to work well. These operations need to happen atomically. That is, they need to be wrapped in transactions. You'll likely need to move this logic to BrowserProvider.

Comment 73

[Jing-wei Wu [:jwu]]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

https://reviewboard.mozilla.org/r/120816/#review125146

> Please add a class comment describing what this is and how it's used.

Ok, I'll add comment and re-send the patch

> Don't mix naming conventions. I don't believe we use hungarian notation much, and certainly not for any new code. Simply `isMobileFolderPrefOn` will do.

Ok, I'll refactor the naming. 

But there are some classes using hungarian notation(for example: HomeFragment.java). Is there any coding guidelines for Fennec that I can prevent this kind of issue happen again?

> Why is this init stuff here and not in BookmarksPanel's onCreateView, as it was before? Doesn't this mean you're now setting up a bookmark-specific observer whenever any HomeFragment instance is created? That doesn't seem right to me.

The purpose of this perf is to enable or disable full-page dialog. And the entry point to open the dialog is in HomeFragment.java(Parent of BookmarksPanel) instead of BookmarksPanel.java

In the method `onContextItemSelected(MenuItem)` in HomeFragment.java, we'll use the pref to decide to show which bookmark dialog.

```java
if (itemId == R.id.home_edit_bookmark) {
    if (mBookmarkPrefs.isEnabled()) {
        // TODO: enter full-page dialog
    } else {
        new EditBookmarkDialog(context).show(info.url);
    }
}
```

Comment 74

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Grisha Kruglov from comment #70)
> Sebastian, could you please help provide some guidance on setting up a
> switchboard experiment on the kinto side of things? Management url in
> https://wiki.mozilla.org/Mobile/Fennec/Android/Downloadable_Content doesn't
> seem to work for me (perhaps Bug 1313038 is helpful).

I used this as an opportunity to update the wiki page for switchboard:
https://wiki.mozilla.org/Mobile/Fennec/Android/Switchboard

You should have been whitelisted for accessing the admin mentioned on the page.

Comment 75

[Jing-wei Wu [:jwu]]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/120810/#review125178

> I don't think this class should exist in its current form. Please see my comment regarding refactoring this, and moving actual editing logic into `BrowserProvider` and friends.

The new patch will remove this class and move the logic into BrowserProvider.

Comment 76

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

https://reviewboard.mozilla.org/r/120812/#review125174

> This doesn't feel right to me.
> 
> There are two major things going on here.
> 
> 2) 1) You have conflated a simple thing, a cache, with a complicated thing - collection of functions to modify bookmarks, folders, etc in live storage. Keep your cache very simple, and move db operations to the data layer. For these data types, we have `BrowserProvider` and `LocalBrowserProvider`, which handle operations on the database - deletions, removals, etc. `BrowserProvider` handles various compound operations, and provides you with ways to perform queries transactionally.
> 
> 2) You have implemented update/remove operations in a very unsafe manner. You should be wrapping these in transactions, and performing them atomically.
> 
> For a very trivial case of why this is bad, consider what will happen if you kill fennec while you're trying to delete a deeply nested folder structure.

The new patch will remove this class and move the logic into LocalBrowserProvider.

> This is duplicated almost verbatim in EditBookmarkTask. What's going on here?

I'll put this code into a utility class to prevent duplication.

> Why all caps? No other string is in all caps.

This string is for Snackbar action text. The material design guide line suggests use all caps for action button. https://material.io/guidelines/components/snackbars-toasts.html#snackbars-toasts-specs

Comment 77

[Jing-wei Wu [:jwu]]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120816/diff/2-3/

Comment 78

[Jing-wei Wu [:jwu]]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120810/diff/2-3/

Comment 79

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/2-3/

Comment 80

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/2-3/

Comment 81

[:Grisha Kruglov]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

https://reviewboard.mozilla.org/r/120816/#review128674

Comment 82

[:Grisha Kruglov]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/120810/#review128724

This kind of approach to restore is not going to work.

Your current approach is, roughly speaking:
- whenever a bookmark is update is requested, store its current state into a cache prior, then run the actual db update
- "restore from cache" then means "flush contents of the cache into the database, updating or inserting records as necessary"

Consider a very simple scenario:
- bookmark X is in state 1
- user is editing bookmark, changes bookmark X to state 2. State 1 is cached.
- afterwards, sync kicks in, and updates bookmark X to state 3 after merging remote and local changes
- user clicks "undo" button. You can't restore Stat 1 at this point and expect good things to happen.

You might be able to do some things to implement "undo" without corrupting data:
- store both "current" and "new" version in the cache. When restore happens, do not restore bookmark if "new" version doesn't match what's in the db.
- kill cache at various times (before/after sync...)...

However, doing any of this correctly in the current system is going to be complicated, if not impossible. Our data models simply do not support this sort of a thing. A system that would make this easy looks quite different, likely involves storing and syncing transaction logs, and might come in a form of something like https://github.com/mozilla/mentat/

I would argue that you should drop this whole patch, and remove this functionality from the UI.

Comment 83

[:Grisha Kruglov]

Richard, NI as a sanity check. Does Comment 82 sound reasonable to you? I can't think of a sane, incremental way to do this sort of a thing (rollback changes to bookmarks) correctly.

Comment 84

[Richard Newman [:rnewman]]

(In reply to :Grisha Kruglov from comment #83)
> Richard, NI as a sanity check. Does Comment 82 sound reasonable to you? I
> can't think of a sane, incremental way to do this sort of a thing (rollback
> changes to bookmarks) correctly.

I agree that implementing restore by re-adding to storage from a cache will be complex and buggy, and that's not the direction to go in; Part 2 as written should be removed.

To expand on your comment, I think there are actually _two_ things to worry about.

The first, as you describe, is racing data. Indeed, the backing data can change _while the editing UI is open_. Whenever possible, avoid having to correctly manage caches and UI state! (As they say, there are two hard problems in computer science: naming things, cache invalidation, and off-by-one errors.)

The second thing to worry about is that a read of, or change to, the database is quite likely to trigger a sync -- Android will do this automatically. Not only does that make race conditions more likely, it also means that a deletion is likely to immediately propagate to other devices. That's a huge can of worms: the restore will trigger a sync, but the rate limiter will kick in, so the restore probably won't be uploaded for another hour. The restored data needs to be marked in order to have the correct effect on remote clients, including in the presence of conflicts -- one cannot simply restore the original database row, but marking it as changed will have an effect on conflict resolution elsewhere.

Even getting this half right requires relatively deep knowledge of both BrowserProvider and Sync on all three platforms. Getting it 100% right would involve quite a bit of time on a whiteboard, if it's even possible, and I would expect to see a thousand lines of tests to make sure it behaves as expected.

There are two ways in which undo could be meaningfully implemented.

One is to do it by flipping a flag in the DB, and only exposing the UI to users without Sync configured. Don't bother with any of the fancy cache stuff.

The other, more complete way is the way email clients offer an "oops!" button to undo a send: hitting Send doesn't actually send the email, it just looks like it does. The send happens later, when there's no chance that you've changed your mind.

If deletion is rephrased as "queue for deletion and pretend it already happened", and this simple queue processed at various points (when the undo UI disappears, or when a sync begins), then it's possible to support something that _looks like_ undo without the data model actually needing to support it.

If this is a feature that we think needs to ship, then that's the way I'd build it.

Comment 85

[:Grisha Kruglov]

Hi Wesley,

Do you think we can ship this feature without "undo" functionality? See Comment 82 and Comment 84. That will significantly simplify things and lower risk around shipping this.

Comment 86

[:Grisha Kruglov]

Same question for product, Joe - please see the few comments above.

Comment 87

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to :Grisha Kruglov from comment #85)
> Hi Wesley,
> 
> Do you think we can ship this feature without "undo" functionality? See
> Comment 82 and Comment 84. That will significantly simplify things and lower
> risk around shipping this.

I checked with Tori on the UX perspective and Joe as well.
As long as we have the follow-up bug to add that "undo" function, we're fine to proceed with the plan.
I think that follow-up bug depends on Bug 1343985 so it makes sense to have it land a bit later.
That said, bookmark management enhancement in 56 could have no "undo". Let's say adding "undo" in 57.

Comment 88

[Jing-wei Wu [:jwu]]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120816/diff/3-4/

Comment 89

[Jing-wei Wu [:jwu]]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120810/diff/3-4/

Comment 90

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/3-4/

Comment 91

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/3-4/

Comment 92

[Jing-wei Wu [:jwu]]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120816/diff/4-5/

Comment 93

[Jing-wei Wu [:jwu]]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120810/diff/4-5/

Comment 94

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/4-5/

Comment 95

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/4-5/

Comment 96

[:Grisha Kruglov]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/120810/#review132216

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1251
(Diff revision 5)
>          cr.registerContentObserver(mBookmarksUriWithProfile, false, observer);
>      }
>  
>      @Override
>      @RobocopTarget
> -    public void updateBookmark(ContentResolver cr, int id, String uri, String title, String keyword) {
> +    public void updateBookmark(ContentResolver cr, long id, String uri, String title, String keyword) {

Why did you switch to `long`? java's ints max out at 2,147,483,647.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1270
(Diff revision 5)
> +    public void updateBookmark(ContentResolver cr, long id, String uri, String title, String keyword, long folderId) {
>          ContentValues values = new ContentValues();
>          values.put(Bookmarks.TITLE, title);
>          values.put(Bookmarks.URL, uri);
>          values.put(Bookmarks.KEYWORD, keyword);
> +        values.put(Bookmarks.PARENT, folderId);

When you're reparenting a bookmarks, you're also changing lists of children of its old parent and its new parent.

So, you'd need to bump lastModified timestamps of both of those records.

That's why we bump parent's record l-m on deletion of one of its children.

During sync, when we upload a bookmark folder record, it includes a list of guids of all of its children.

Note that you'd only want to bump those timestamps if you did reparent a record.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1829
(Diff revision 5)
>          return c;
>      }
>  
>      @Override
> +    public Cursor getBookmarkById(ContentResolver cr, long id) {
> +        Cursor c = cr.query(bookmarksUriWithLimit(1),

You shouldn't need limit if you're querying by id. There won't be any duplicates.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/LocalBrowserDBTest.java:73
(Diff revision 5)
> +        assertNotEquals(bookmarkId, INVALID_ID);
> +
> +        // Remove bookmark record
> +        db.removeBookmarkWithId(cr, bookmarkId);
> +
> +        cursor = db.getBookmarkForUrl(cr, BOOKMARK_URL);

Test that you bump parent's l-m timestamp.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/LocalBrowserDBTest.java:112
(Diff revision 5)
> +
> +            final String updatedUrl = cursor.getString(cursor.getColumnIndexOrThrow(BrowserContract.Bookmarks.URL));
> +            assertEquals(updatedUrl, UPDATE_URL);
> +
> +            final String updatedTitle = cursor.getString(cursor.getColumnIndexOrThrow(BrowserContract.Bookmarks.TITLE));
> +            assertEquals(updatedTitle, UPDATE_TITLE);

You'd also want to test:
- ability to change parent id
- that you bump l-m timestamps on update of both old and new parents if you're performing reparenting.

so you probably want to test a few update scenarios here, with different side-effects.

Comment 97

[:Grisha Kruglov]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/120810/#review132228

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/LocalBrowserDBTest.java:1
(Diff revision 5)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Tests are public domain. For a sample header see another test file.

Comment 98

[Jing-wei Wu [:jwu]]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/120810/#review132216

> Why did you switch to `long`? java's ints max out at 2,147,483,647.

There are some methods in BrowserDB.java use `long` as the data type for bookmark folder ID. For example: 

public abstract Cursor getBookmarksInFolder(ContentResolver cr, long folderId);
public abstract int getBookmarkCountForFolder(ContentResolver cr, long folderId);
            
Since folder is a kind of bookmark, which means id(Bookmarks._ID) and folderId(Bookmarks.PARENT) should have same data type, the change here is for type consistency.

Comment 99

[Jing-wei Wu [:jwu]]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120816/diff/5-6/

Comment 100

[Jing-wei Wu [:jwu]]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120810/diff/5-6/

Comment 101

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/5-6/

Comment 102

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/5-6/

Comment 103

[Jing-wei Wu [:jwu]]

Comment on attachment 8847887 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120816/diff/6-7/

Comment 104

[Jing-wei Wu [:jwu]]

Comment on attachment 8847888 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120810/diff/6-7/

Comment 105

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/6-7/

Comment 106

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/6-7/

Comment 107

[Andrzej Hunt :ahunt]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

https://reviewboard.mozilla.org/r/120812/#review133538

THe UI parts look good to me - I'm relying on Grisha for the DB/bookmark parts!

::: mobile/android/base/resources/values/themes.xml:136
(Diff revision 7)
>      </style>
>  
>      <style name="GeckoCustomTabs.Light" parent="Theme.AppCompat.Light.NoActionBar">
>          <item name="menuItemActionBarStyle">@style/Widget.MenuItemActionBar</item>
>          <item name="menuItemDefaultStyle">@style/Widget.MenuItemCustomTabs</item>
> -    </style>
> +	    </style>

nit: looks like htis might be an uninteded indentation change?

Comment 108

[Andrzej Hunt :ahunt]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

https://reviewboard.mozilla.org/r/120814/#review133540

::: mobile/android/base/resources/values/dimens.xml:58
(Diff revision 7)
>      <dimen name="tablet_browser_toolbar_menu_item_inset_vertical">5dp</dimen>
>      <dimen name="tablet_browser_toolbar_menu_item_inset_horizontal">3dp</dimen>
>      <dimen name="tablet_tab_strip_button_inset">5dp</dimen>
>  
>      <!-- Dimensions used by Favicons and FaviconView -->
> -    <dimen name="favicon_bg">32dp</dimen>
> +    <dimen name="favicon_bg">25dp</dimen>

I'm slightly concerned about changing this - it also affects things like:
- Preferences (it's used for search engine preferences)
- Activity Stream context menu
- Media Control notification

I'm wondering if we should create a new dimension to use for the homepanels, vs keeping the existing dimension for the other uses?

Comment 109

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/7-8/

Comment 110

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/7-8/

Comment 111

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

https://reviewboard.mozilla.org/r/120814/#review133540

> I'm slightly concerned about changing this - it also affects things like:
> - Preferences (it's used for search engine preferences)
> - Activity Stream context menu
> - Media Control notification
> 
> I'm wondering if we should create a new dimension to use for the homepanels, vs keeping the existing dimension for the other uses?

Yes I should create a new dimension(favicon_small_size) to prevent side effect for others.

Comment 112

[Jing-wei Wu [:jwu]]

Attachment: Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

The experiment in Switchboard is "full-bookmark-management", and it's enabled for 100% of the Nightly audience.

Review commit: https://reviewboard.mozilla.org/r/131010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131010/

Comment 113

[Jing-wei Wu [:jwu]]

Attachment: Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review commit: https://reviewboard.mozilla.org/r/131012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131012/

Comment 114

[:Grisha Kruglov]

Comment on attachment 8859000 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/131012/#review133928

This is getting pretty close! I think another pass and we're there.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1521
(Diff revision 1)
>                         " FROM " + TABLE_BOOKMARKS +
>                         " WHERE " + selection + " )";
>          return db.update(TABLE_BOOKMARKS, values, where, selectionArgs);
>      }
>  
>      private long insertBookmark(Uri uri, ContentValues values) {

Generally I think I'm fine with moving this logic here.

It's important you don't perform these updates if insert is happening from sync. Sync will handle timestamps on its own, and so this is only necessary for updates from within fennec itself.

However, beware that LocalBrowserDB's `addBookmarkItem` already does this! And so, you will need to unite the two. This could be as simple as removing LocalBrowserDB's bits, adding comments that the underlying content provider will handle timestamp trickery.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1555
(Diff revision 1)
>  
>          debug("Inserting bookmark in database with URL: " + url);
>          final SQLiteDatabase db = getWritableDatabase(uri);
>          beginWrite(db);
> -        return db.insertOrThrow(TABLE_BOOKMARKS, Bookmarks.TITLE, values);
> +        final long id = db.insertOrThrow(TABLE_BOOKMARKS, Bookmarks.TITLE, values);
> +        if (id >= 0) {

Prefer early returns.

So change this to if (insertedId == -1) {log error and return}

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1560
(Diff revision 1)
> +        if (id >= 0) {
> +            // Bump parent's lastModified timestamp.
> +            final long lastModified = values.getAsLong(Bookmarks.DATE_MODIFIED);
> +            final ContentValues parentValues = new ContentValues();
> +            parentValues.put(Bookmarks.DATE_MODIFIED, lastModified);
> +            updateBookmarkParents(db, parentValues, Bookmarks._ID + " = ?", new String[] { String.valueOf(id) });

Most likely you will have parent id available in the `values`, right? In that case you can just bump that record directly, as opposed to performing another `select` in the method call.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1613
(Diff revision 1)
>          }
>  
>          beginWrite(db);
> -        return db.update(TABLE_BOOKMARKS, values, inClause, null);
> +
> +        final boolean isParentChanged = isBookmarkParentChanged(uri);
> +        final boolean isDeleted = values.containsKey(Bookmarks.IS_DELETED) && values.getAsBoolean(Bookmarks.IS_DELETED);

We won't be removing bookmarks via a call to update. See implementation of `deleteBookmarks` for what a delete actually is - in short, it's not enough to just set the IS_DELETED flag.

This means you should kill the `isDeleted` var and its uses.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1623
(Diff revision 1)
> +            final ContentValues parentValues = new ContentValues();
> +            parentValues.put(Bookmarks.DATE_MODIFIED, now);
> +            updateBookmarkParents(db, parentValues, inClause, null);
> +        }
> +
> +        int updated = db.update(TABLE_BOOKMARKS, values, inClause, null);

final.

Also, this could be 0, right? In that case you probably want to log and early return.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1629
(Diff revision 1)
> +
> +        // Bump new parent's lastModified timestamp.
> +        if (isParentChanged) {
> +            final ContentValues parentValues = new ContentValues();
> +            parentValues.put(Bookmarks.DATE_MODIFIED, now);
> +            updateBookmarkParents(db, parentValues, inClause, null);

You know the new parent id since you've just updated it. Just do a direct update of that record here, don't go through `updateBookmarkParents`.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1276
(Diff revision 1)
>                    new String[] { String.valueOf(id) });
>      }
>  
>      @Override
> +    public void updateBookmark(ContentResolver cr, long id, String uri, String title, String keyword, long parentId) {
> +        ContentValues values = new ContentValues();

final

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1283
(Diff revision 1)
> +        values.put(Bookmarks.URL, uri);
> +        values.put(Bookmarks.KEYWORD, keyword);
> +        values.put(Bookmarks.PARENT, parentId);
> +        values.put(Bookmarks.DATE_MODIFIED, System.currentTimeMillis());
> +
> +        Uri contentUri = mBookmarksUriWithProfile.buildUpon()

final

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1827
(Diff revision 1)
>                                             Bookmarks.URL,
>                                             Bookmarks.TITLE,
> +                                           Bookmarks.TYPE,
> +                                           Bookmarks.PARENT,
> +                                           Bookmarks.GUID,
>                                             Bookmarks.KEYWORD },

Double check that any users of this method don't depend on the positions of columns or their number. e.g. they'll always query the cursor by the column name KEYWORD.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1842
(Diff revision 1)
>          return c;
>      }
>  
>      @Override
> +    public Cursor getBookmarkById(ContentResolver cr, long id) {
> +        Cursor c = cr.query(mBookmarksUriWithProfile,

final

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/LocalBrowserDBTest.java:237
(Diff revision 1)
> +        }
> +
> +        // Check parent's lastModified timestamp is updated
> +        final long lastModifiedAfterAdd = getModifiedDate(rootFolderId);
> +        assertTrue(lastModifiedAfterAdd > lastModifiedBeforeAdd);
> +    }

you've also modified simple bookmark insertion - so, make sure you're testing those changes as well. Above you're testing that code path for folders, i'd add an explicit test for non-folders too.

Comment 115

[:Grisha Kruglov]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

https://reviewboard.mozilla.org/r/120812/#review132270

I trust ahunt's judgement on the UI aspects of this, and I've generally glanced through the code. General nit comment: please go through your comments and make sure they're full sentences with good grammar.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:4184
(Diff revision 5)
>              Telemetry.sendUIEvent(TelemetryContract.Event.LAUNCH, method, "restricted");
>          }
>      }
> +
> +    /**
> +     * Launch edit bookmark dialog. The {@link BookmarkEditFragment} needs to be started by the Activity

"by an activity"

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:36
(Diff revision 8)
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.db.BrowserContract.Bookmarks;
> +import org.mozilla.gecko.db.BrowserDB;
> +
> +/**
> + * A dialog fragment that allows to edit a bookmark url, title and change parent folder.

"... that allows editing bookmark's url, title and changing the parent."

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:44
(Diff revision 8)
> +
> +    private static final String ARG_ID = "id";
> +    private static final String ARG_URL = "url";
> +    private static final String ARG_BOOKMARK = "bookmark";
> +
> +    private long mId;

nit, but please don't use this notation. It adds no value and makes this code harder to glance through. Just `bookmark`, `url` etc is fine.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:72
(Diff revision 8)
> +        args.putLong(ARG_ID, id);
> +        fragment.setArguments(args);
> +        return fragment;
> +    }
> +
> +    public static BookmarkEditFragment newInstance(String url) {

nit, but don't repeat yourself. Probably cleaner to add a third method which takes a bundle.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:214
(Diff revision 8)
> +
> +        // Enable menu item after bookmark is set to view
> +        final MenuItem doneItem = mToolbar.getMenu().findItem(R.id.done);
> +        doneItem.setEnabled(true);
> +
> +        // Create our TextWatchers

nit: comments should be "why", not "what". I see from the code _what_ you're doing, but comments should explain _why_ you're doing it.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:243
(Diff revision 8)
> +        final String originalUrl;
> +        final long originalParentId;
> +        final String originalFolder;
> +
> +        // Can be modified in this fragment.
> +        String title;

Prefer immutability whenever possible. Ensure you have good reasons for modifying objects like this.

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:300
(Diff revision 8)
> +                final long originalParentId = source.readLong();
> +                final String originalFolder = source.readString();
> +
> +                final Bookmark bookmark = new Bookmark(id, originalUrl, originalTitle, keyword,
> +                                                       originalParentId, originalFolder, type, guid);
> +                bookmark.url = modifiedUrl;

Why is this necessary? Why can't you just create bookmark object above with correct values?

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:457
(Diff revision 8)
> +            bookmark = null;
> +        }
> +    }
> +
> +    /**
> +     * This text watcher to enable or disable the menu item if the dialog contains

nit: grammar

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:462
(Diff revision 8)
> +     * This text watcher to enable or disable the menu item if the dialog contains
> +     * valid information.
> +     */
> +    private class BookmarkTextWatcher implements TextWatcher {
> +        // A stored reference to the dialog containing the text field being watched
> +        private MenuItem doneItem;

Can the watcher outlive the menuItem? should this be a weakreference?

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkEditFragment.java:462
(Diff revision 8)
> +     * This text watcher to enable or disable the menu item if the dialog contains
> +     * valid information.
> +     */
> +    private class BookmarkTextWatcher implements TextWatcher {
> +        // A stored reference to the dialog containing the text field being watched
> +        private MenuItem doneItem;

final

::: mobile/android/base/java/org/mozilla/gecko/bookmark/EditBookmarkTask.java:46
(Diff revision 8)
> +        final String url = bundle.getString(BrowserContract.Bookmarks.URL);
> +        final String title = bundle.getString(BrowserContract.Bookmarks.TITLE);
> +        final String keyword = bundle.getString(BrowserContract.Bookmarks.KEYWORD);
> +
> +        if (bundle.containsKey(BrowserContract.Bookmarks.PARENT)) {
> +            final long parentId = bundle.getLong(BrowserContract.Bookmarks.PARENT);

make sure there is no chance that the new parent id is the same as the old parent id. It will cause _a lot_ of unnecessary work - db writes, more to do during a sync, more to do on other clients you're syncing with, etc - if you get this wrong.

Generally any screw ups that you might get away with locally are amplified ten fold when you start syncing their consequences and other clients start to act upon them. So beware.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarksPanel.java:183
(Diff revision 8)
> +        ContextMenuInfo menuInfo = item.getMenuInfo();
> +        if (!(menuInfo instanceof HomeContextMenuInfo)) {
> +            return false;
> +        }
> +
> +        HomeContextMenuInfo info = (HomeContextMenuInfo) menuInfo;

Here and elsewhere - are you going to be changing references? If not, mark them as final.

::: mobile/android/base/resources/layout/bookmark_edit2.xml:1
(Diff revision 8)
> +<?xml version="1.0" encoding="utf-8"?>

Maybe this file should be named better?

Comment 116

[:Grisha Kruglov]

Comment on attachment 8858999 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

https://reviewboard.mozilla.org/r/131010/#review133978

::: mobile/android/base/java/org/mozilla/gecko/bookmark/BookmarkUtils.java:6
(Diff revision 1)
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.bookmark;

Name this `.bookmarks`.

Comment 117

[Jing-wei Wu [:jwu]]

Comment on attachment 8858999 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131010/diff/1-2/

Comment 118

[Jing-wei Wu [:jwu]]

Comment on attachment 8859000 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131012/diff/1-2/

Comment 119

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/8-9/

Comment 120

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/8-9/

Comment 121

[Jing-wei Wu [:jwu]]

Comment on attachment 8859000 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/131012/#review133928

> Generally I think I'm fine with moving this logic here.
> 
> It's important you don't perform these updates if insert is happening from sync. Sync will handle timestamps on its own, and so this is only necessary for updates from within fennec itself.
> 
> However, beware that LocalBrowserDB's `addBookmarkItem` already does this! And so, you will need to unite the two. This could be as simple as removing LocalBrowserDB's bits, adding comments that the underlying content provider will handle timestamp trickery.

Sync will handle timestamps on its own for all db operations(insert/update/delete), right?

I'll use `isCallerSync()` to check if it's triggered from sync.
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/SQLiteBridgeContentProvider.java#113

Comment 122

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

https://reviewboard.mozilla.org/r/120812/#review132270

> Prefer immutability whenever possible. Ensure you have good reasons for modifying objects like this.

These variables are mutable. When onSaveInstanceState() is triggered, we fetch the UI changes and save into them.

When fragment is re-created, we can use these values to restore UI.

> make sure there is no chance that the new parent id is the same as the old parent id. It will cause _a lot_ of unnecessary work - db writes, more to do during a sync, more to do on other clients you're syncing with, etc - if you get this wrong.
> 
> Generally any screw ups that you might get away with locally are amplified ten fold when you start syncing their consequences and other clients start to act upon them. So beware.

I leave the check in BookmarkEditFragment.java
```
if (bookmark.parentId != bookmark.originalParentId) {
    bundle.putLong(Bookmarks.PARENT, bookmark.parentId);
}
```

In EditBookamrk.java, it doesn't update parent field in database if the bundle don't have the key 'PARENT'.

Comment 123

[:Grisha Kruglov]

Comment on attachment 8859000 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/131012/#review134446

One important thing to change, and various nits around code itself.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1570
(Diff revision 2)
> +
> +        // Bump parent's lastModified timestamp.
> +        final long lastModified = values.getAsLong(Bookmarks.DATE_MODIFIED);
> +        final ContentValues parentValues = new ContentValues();
> +        parentValues.put(Bookmarks.DATE_MODIFIED, lastModified);
> +        final long parentId = values.getAsLong(Bookmarks.PARENT);

We can also be inserting a root folder without a parent, right? In which case there wouldn't be a parentId, and so there wouldn't be anything to do here.

Make sure your unit tests handle these variations (root vs non-root).

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1623
(Diff revision 2)
>              cursor.close();
>          }
>  
>          beginWrite(db);
> -        return db.update(TABLE_BOOKMARKS, values, inClause, null);
> +
> +        final boolean isCallerSync = isCallerSync(uri);

Add a comment describing why what you're doing below is necessary. Explain that this is needed so that during a sync we know to upload folders whose lists of children changed.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1625
(Diff revision 2)
>  
>          beginWrite(db);
> -        return db.update(TABLE_BOOKMARKS, values, inClause, null);
> +
> +        final boolean isCallerSync = isCallerSync(uri);
> +        final boolean isParentChanged = isBookmarkParentChanged(uri);
> +        final long now = values.getAsLong(Bookmarks.DATE_MODIFIED);

`now` is a bad name for this. Perhaps `newLastModified`?

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1627
(Diff revision 2)
> -        return db.update(TABLE_BOOKMARKS, values, inClause, null);
> +
> +        final boolean isCallerSync = isCallerSync(uri);
> +        final boolean isParentChanged = isBookmarkParentChanged(uri);
> +        final long now = values.getAsLong(Bookmarks.DATE_MODIFIED);
> +
> +        // Bump old parent's lastModified timestamp before record updated.

I don't think the order here matters. The end result is that lastModified timestamps for both the new parent, old parent and the child will be the same after your updates.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1629
(Diff revision 2)
> +        final boolean isParentChanged = isBookmarkParentChanged(uri);
> +        final long now = values.getAsLong(Bookmarks.DATE_MODIFIED);
> +
> +        // Bump old parent's lastModified timestamp before record updated.
> +        if (!isCallerSync && isParentChanged) {
> +            final ContentValues parentValues = new ContentValues();

`oldParentValues`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1636
(Diff revision 2)
> +            updateBookmarkParents(db, parentValues, inClause, null);
> +        }
> +
> +        final int updated = db.update(TABLE_BOOKMARKS, values, inClause, null);
> +        if (updated == 0) {
> +            trace("No update on URI: " + uri);

If nothing changed, for some reason, you don't want to bump parent. Re-order this.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1641
(Diff revision 2)
> +            trace("No update on URI: " + uri);
> +            return updated;
> +        }
> +
> +        // Bump new parent's lastModified timestamp.
> +        if (!isCallerSync && isParentChanged) {

After you re-order the above code, you should be able to do an early return on `isCallerSync`, and unite the two if blocks. This will make the function much more readable at a glance.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1642
(Diff revision 2)
> +            return updated;
> +        }
> +
> +        // Bump new parent's lastModified timestamp.
> +        if (!isCallerSync && isParentChanged) {
> +            final ContentValues parentValues = new ContentValues();

`newParentValues`

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1645
(Diff revision 2)
> +        // Bump new parent's lastModified timestamp.
> +        if (!isCallerSync && isParentChanged) {
> +            final ContentValues parentValues = new ContentValues();
> +            parentValues.put(Bookmarks.DATE_MODIFIED, now);
> +
> +            final long parentId = values.getAsLong(Bookmarks.PARENT);

call it `newParentId` to be super obvious.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
(Diff revision 2)
>      @RobocopTarget
>      public void removeBookmarksWithURL(ContentResolver cr, String uri) {
> -        Uri contentUri = mBookmarksUriWithProfile;
> -
> -        // Do this now so that the items still exist!
> -        bumpParents(cr, Bookmarks.URL, uri);

I think it should be possible to remove `bumpParents` as well now, and remove the parentsUri interface on the BrowserProvider, if no one else is using it anymore.

If we're moving this bumping logic to internals of BrowserProvider, might as well go all the way if possible.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1230
(Diff revision 2)
> -        bumpParents(cr, Bookmarks.URL, uri);
> -
>          final String[] urlArgs = new String[] { uri, String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) };
>          final String urlEquals = Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " != ? ";
>  
> -        cr.delete(contentUri, urlEquals, urlArgs);
> +        cr.delete(mBookmarksUriWithProfile, urlEquals, urlArgs);

Be consistent in your two delete remove methods. They look different at a glance but largely do the same thing. I'd just drop these variables.

Comment 124

[Jing-wei Wu [:jwu]]

Comment on attachment 8859000 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/131012/#review134446

> We can also be inserting a root folder without a parent, right? In which case there wouldn't be a parentId, and so there wouldn't be anything to do here.
> 
> Make sure your unit tests handle these variations (root vs non-root).

I don't think we can insert a folder without a parent. 

1. To create a folder, the API `addBookmarkFolder` in BrowserDB.java needs caller to provide parent id.
2. To create a bookmark, it's always insert to root parent whose guid is `Bookmarks.MOBILE_FOLDER_GUID`.

> I don't think the order here matters. The end result is that lastModified timestamps for both the new parent, old parent and the child will be the same after your updates.

Since the ContentValues only has new parent id, I'll do a little refactor that put old parent id into the uri's query parameter.

Then we can update lastModified timestamps for both new parent and old parent at once.

> I think it should be possible to remove `bumpParents` as well now, and remove the parentsUri interface on the BrowserProvider, if no one else is using it anymore.
> 
> If we're moving this bumping logic to internals of BrowserProvider, might as well go all the way if possible.

I'll remove `bumpParents` since Android Studio said no one calls this method.

But for removing parentsUri interface, maybe we should file another bug for refactoring(with some checks and test cases) since this is a litte out of scope?

Comment 125

[Jing-wei Wu [:jwu]]

Comment on attachment 8858999 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131010/diff/2-3/

Comment 126

[Jing-wei Wu [:jwu]]

Comment on attachment 8859000 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131012/diff/2-3/

Comment 127

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/9-10/

Comment 128

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/9-10/

Comment 129

[:Grisha Kruglov]

Comment on attachment 8859000 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

https://reviewboard.mozilla.org/r/131012/#review135042

Looks good. Make sure that our non-unit tests all pass, and please test this extensively.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1567
(Diff revision 3)
> +
> +        // Bump parent's lastModified timestamp.
> +        final long lastModified = values.getAsLong(Bookmarks.DATE_MODIFIED);
> +        final ContentValues parentValues = new ContentValues();
> +        parentValues.put(Bookmarks.DATE_MODIFIED, lastModified);
> +        final long parentId = values.getAsLong(Bookmarks.PARENT);

Add a comment here explaining why values will always have a parent, and why you don't need an explicit check.

::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1568
(Diff revision 3)
> +        // Bump parent's lastModified timestamp.
> +        final long lastModified = values.getAsLong(Bookmarks.DATE_MODIFIED);
> +        final ContentValues parentValues = new ContentValues();
> +        parentValues.put(Bookmarks.DATE_MODIFIED, lastModified);
> +        final long parentId = values.getAsLong(Bookmarks.PARENT);
> +        db.update(TABLE_BOOKMARKS, parentValues, Bookmarks._ID + " = ?", new String[] { String.valueOf(parentId) });

nit: break this into multiple lines

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1163
(Diff revision 3)
>                    values,
>                    Bookmarks.URL + " = ? AND " +
>                    Bookmarks.PARENT + " = " + folderId,
>                    new String[] { uri });
>  
> -        // Bump parent modified time using its ID.
> +        // BrowserProvider will handle updating parent's lastModified timestamp, nothing need to do here.

"nothing else to do."

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1207
(Diff revision 3)
> +        values.put(Bookmarks.DATE_MODIFIED, now);
> +        values.put(Bookmarks.GUID, Utils.generateGuid());
> +        values.put(Bookmarks.PARENT, parentId);
> +        values.put(Bookmarks.TITLE, title);
> +        values.put(Bookmarks.TYPE, Bookmarks.TYPE_FOLDER);
> +        return cr.insert(mBookmarksUriWithProfile, values);

Add a comment mentioning that BrowserProvider will bump parent here as well.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1213
(Diff revision 3)
> +    }
> +
> +    @Override
>      @RobocopTarget
>      public void removeBookmarksWithURL(ContentResolver cr, String uri) {
> -        Uri contentUri = mBookmarksUriWithProfile;
> +        cr.delete(mBookmarksUriWithProfile,

Add a comment mentioning that BrowserProvider will bump parents.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1213
(Diff revision 3)
> +    }
> +
> +    @Override
>      @RobocopTarget
>      public void removeBookmarksWithURL(ContentResolver cr, String uri) {
> -        Uri contentUri = mBookmarksUriWithProfile;
> +        cr.delete(mBookmarksUriWithProfile,

Note that you did subtly change the behaviour here. Specifically the selection criteria for bumping parents are different. Which means we won't bump l-m timestamp of the special FIXED_PINNED_LIST folder. Which is probably more correct than the previous behaviour - when we did bump it, but wouldn't delete the record itself if it turned out to be a pin.

I don't think we sync that folder anyway, so I'm not sure this matters anyway, but make sure this won't regress behaviour of pinned sites on some weird way.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1220
(Diff revision 3)
> -        final String[] urlArgs = new String[] { uri, String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) };
> -        final String urlEquals = Bookmarks.URL + " = ? AND " + Bookmarks.PARENT + " != ? ";
>  
> -        cr.delete(contentUri, urlEquals, urlArgs);
> +    @Override
> +    public void removeBookmarkWithId(ContentResolver cr, long id) {
> +        cr.delete(mBookmarksUriWithProfile,

Same comment about bumping parents

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1246
(Diff revision 3)
>                    Bookmarks._ID + " = ?",
>                    new String[] { String.valueOf(id) });
>      }
>  
>      @Override
> +    public void updateBookmark(ContentResolver cr, long id, String uri, String title, String keyword,

Just make sure this API isn't awkward to use from the other end. It's a good thing you're writing a consumer for this as well.

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1255
(Diff revision 3)
> +        values.put(Bookmarks.URL, uri);
> +        values.put(Bookmarks.KEYWORD, keyword);
> +        values.put(Bookmarks.PARENT, newParentId);
> +        values.put(Bookmarks.DATE_MODIFIED, System.currentTimeMillis());
> +
> +        final Uri contentUri = mBookmarksUriWithProfile.buildUpon()

You could also pass it via contentvalues above, but this works as well.

Comment 130

[Jing-wei Wu [:jwu]]

Comment on attachment 8858999 [details]
Bug 1232439 - Part 1: Setup A/B experiment for enabling full bookmark management in Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131010/diff/3-4/

Comment 131

[Jing-wei Wu [:jwu]]

Comment on attachment 8859000 [details]
Bug 1232439 - Part 2: Support get/update/remove bookmark record by ID in BrowserDB.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131012/diff/3-4/

Comment 132

[Jing-wei Wu [:jwu]]

Comment on attachment 8847889 [details]
Bug 1232439 - Part 3: Implement full-page bookmark edit dialog.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120812/diff/10-11/

Comment 133

[Jing-wei Wu [:jwu]]

Comment on attachment 8847890 [details]
Bug 1232439 - Part 4: Adjust bookmark list UI layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120814/diff/10-11/

Comment 134

[Jing-wei Wu [:jwu]]

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe3262ac57de5584bae5d08edd20945812c1687c

Btw the job name 'android-api-15-frontend' should rename to 'android-test'.
https://bugzilla.mozilla.org/show_bug.cgi?id=1260874

Comment 135

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b8079a1c135
Part 1: Setup A/B experiment for enabling full bookmark management in Nightly. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/59af50c06ae5
Part 2: Support get/update/remove bookmark record by ID in BrowserDB. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/5a0b0722bb13
Part 3: Implement full-page bookmark edit dialog. r=ahunt,Grisha
https://hg.mozilla.org/integration/autoland/rev/0cbed2ce192a
Part 4: Adjust bookmark list UI layout. r=ahunt

Comment 136

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3b8079a1c135
https://hg.mozilla.org/mozilla-central/rev/59af50c06ae5
https://hg.mozilla.org/mozilla-central/rev/5a0b0722bb13
https://hg.mozilla.org/mozilla-central/rev/0cbed2ce192a

Comment 137

[Tori Chen [:tori]]

Clear this needinfo since ux spec is delivered.