Re-engineer edit screen in enhanced bookmark

Re-engineer edit screen in enhanced bookmark

The previous detail activity has been redesigned to a much simpler
model, which only contains basic editing funcions. This CL removes
the the detail activity (marvelous-looking as it is), and impelements
a newer activity that only simple editing.

BUG=499415
Committed: https://crrev.com/474f656d3f477d51f49b56bb17125dccd7d035cf
Cr-Commit-Position: refs/heads/master@{#335283}

[Ian Wen, 2015-06-17 00:30:17]

ianwen@chromium.org changed reviewers:
+ kkimlabs@chromium.org, newt@chromium.org

[Ian Wen, 2015-06-17 00:30:17]

The empty-alert logic has been encapsulated into a single ui widget in
https://codereview.chromium.org/1187013002/, which I plan to use throughout
various views/activities in EnhancedBookmark.

[newt (away), 2015-06-17 04:42:23]

Additional comments:
1. The commit message contains a few typos
2. There doesn't seem to be any validation currently. Are you planning to add
some? The edit activity doesn't have Save/Cancel buttons, so we're depending on
the user to press back, which will save and exit, right? What happens if the
user tries to save a URL with an empty title or URL?

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File chrome/android/java_staging/res/layout/eb_edit.xml (right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:1: <ScrollView
xmlns:android="http://schemas.android.com/apk/res/android"
copyright

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:10:
android:layout_marginTop="8dp"
why not margin bottom?

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:34:
android:textAppearance="@android:style/TextAppearance.Small" />
I believe TextAppearance.Small and TextAppearance.Medium look different on pre-L
vs L devices. You should check and see if these are correct for your needs on
both L and pre-L devices. If not, you can set the size and color explicitly, or
perhaps use a style from AppCompat.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:40: android:clickable="true"
FYI, you don't need to set clickable="true". If you apply a click listener in
Java code, the clickable attribute will automatically be set to true.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File chrome/android/java_staging/res/values-v17/styles_staging.xml (right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/values-v17/styles_staging.xml:78: <style
name="EnhancedBookmarkDialogWhenLarge"
parent="Theme.AppCompat.Light.DialogWhenLarge">
This is identical to EnhancedBookmarkDialogWhite. Why do you need this?

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:22:
* The activity that enables the user to modify the title, url, parent folder of
a bookmark.
"and parent folder"

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:25:
public static final String INTENT_BOOKMARK_ID =
"EnhancedBookmarkEditActivity.BookmarkId";
document public constants

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:33:
private MenuItem mDeleteBtn;
s/Btn/Button

Avoid abbrvtns

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:83:
BookmarkItem bookmarkItem =
mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);
might this ever return null?

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:89:
@Override
fix indentation

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:99:
mDeleteBtn = menu.add(0, 0, 0, "delete").setIcon(R.drawable.btn_trash);
For the sake of accessibility users, use a localized string:

    menu.add(R.string.delete)...

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:106:
if (item.equals(mDeleteBtn)) {
use == instead of equals(), since we only care about identity, not equality.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:121:
mEnhancedBookmarksModel.setBookmarkTitle(mBookmarkId, newTitle);
why don't you validate newTitle and newUrl?

[Kibeom Kim (inactive), 2015-06-17 06:10:30]

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File chrome/android/java_staging/res/layout/eb_edit.xml (right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:13:
<org.chromium.chrome.browser.widget.FloatLabelLayout
Maybe TODO or a bug for TextInputLayout migration in design support library
http://android-developers.blogspot.com/2015/05/android-design-support-library...
?

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File chrome/android/java_staging/res/values-v17/styles_staging.xml (right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/values-v17/styles_staging.xml:78: <style
name="EnhancedBookmarkDialogWhenLarge"
parent="Theme.AppCompat.Light.DialogWhenLarge">
On 2015/06/17 04:42:22, newt wrote:
> This is identical to EnhancedBookmarkDialogWhite. Why do you need this?

I guess it's because in this case we want the top action bar? Though name
doesn't reflect it. Also, is there a reason not to use DialogWhenLarge style?
Seems like it's identical just except android:statusBarColor and
android:textColorLink.

If both are true, then I suggest something like:

<style name="DialogWhenLarge" parent="Theme.AppCompat.Light.DialogWhenLarge" >
    <item name="android:windowBackground">@drawable/bg_white_dialog</item>
    <item name="android:statusBarColor">@android:color/black</item>
    <item name="android:textColorLink">@color/light_active_color</item>
    <item name="colorPrimary">@color/light_active_color</item>
    <item name="colorAccent">@color/light_active_color</item>
</style>

<style name="DialogWhenLargeNoTitleActionBar" parent="DialogWhenLarge" >
    <!-- Remove ActionBar -->
    <item name="windowNoTitle">true</item>
    <item name="windowActionBar">false</item>
</style>

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:56:
if (mBookmarkId.equals(node.getId())) updateViewContent();
Q: what happens if parent node's name is changed?

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:83:
BookmarkItem bookmarkItem =
mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);
On 2015/06/17 04:42:23, newt wrote:
> might this ever return null?

I don't know if it's possible for a bookmark to be deleted between
EnhancedBookmarkEditActivity launching and here. Overly defensive null checking
seems considered harmful
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/KJdw-QiKdxY
. How about landing as it is and monitoring crash reports?

Or, I'm not sure, but maybe this can happen when 1. user opens this activity 2.
killed in background due to memory pressure. 3. user clears app data 4. Come
back to this activity.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:88:
mFolderTextView.setOnClickListener(new View.OnClickListener() {
I guess we don't need to set listener on every update? Maybe in onCreate?

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:99:
mDeleteBtn = menu.add(0, 0, 0, "delete").setIcon(R.drawable.btn_trash);
Seems like Menu.None is recommended instead of 0.
http://developer.android.com/reference/android/view/Menu.html#add(int, int, int,
java.lang.CharSequence)

Optional: This is all static, so I think it's a good place to use menu xml. But
since this is only one I'll leave up to you.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/BookmarkItemView.java
(right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/BookmarkItemView.java:224:
mIsFolder ? R.string.contextmenu_edit_folder : R.string.edit_bookmark)
I think we should also update the comment for IDS_EDIT_BOOKMARK string.
Currently it's "Title of the "Edit Bookmark" form dialog"

[newt (away), 2015-06-17 17:31:17]

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:99:
mDeleteBtn = menu.add(0, 0, 0, "delete").setIcon(R.drawable.btn_trash);
> Optional: This is all static, so I think it's a good place to use menu xml.
But
> since this is only one I'll leave up to you.

My personal preference is to avoid menu xml with a single menu item. IMO, the
xml reduces clarity due to the extra indirection. Either way is fine though.

[Ian Wen, 2015-06-17 21:07:42]

lots of comments. :O Mostly all done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File chrome/android/java_staging/res/layout/eb_edit.xml (right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:1: <ScrollView
xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/06/17 04:42:22, newt wrote:
> copyright

Done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:10:
android:layout_marginTop="8dp"
On 2015/06/17 04:42:22, newt wrote:
> why not margin bottom?

Because on tablet the size of the dialog is fixed and setting margin won't
affect its size. On phone 99% of the devices are large enough to hold the simple
content.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:13:
<org.chromium.chrome.browser.widget.FloatLabelLayout
On 2015/06/17 06:10:30, Kibeom Kim wrote:
> Maybe TODO or a bug for TextInputLayout migration in design support library
>
http://android-developers.blogspot.com/2015/05/android-design-support-library...
> ?

I knew this package exists. However it might not be this CL's responsibility to
touch it. Adding the design support library to chrome might increase the apk
size and it should be done with discretion. Even if we decide to do it later,
FloatLabelLayout will be deleted completely and autofill UIs will need to be
updated as well (see the javadoc for FloatLabelLayout). I think twellington has
a bug to clean this class up.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:34:
android:textAppearance="@android:style/TextAppearance.Small" />
On 2015/06/17 04:42:22, newt wrote:
> I believe TextAppearance.Small and TextAppearance.Medium look different on
pre-L
> vs L devices. You should check and see if these are correct for your needs on
> both L and pre-L devices. If not, you can set the size and color explicitly,
or
> perhaps use a style from AppCompat.

Switched to TextAppearance.AppCompat.Small

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:40: android:clickable="true"
On 2015/06/17 04:42:22, newt wrote:
> FYI, you don't need to set clickable="true". If you apply a click listener in
> Java code, the clickable attribute will automatically be set to true.

I added it to experiment whether setting clickable will give me a color state
list for free. The answer was no and I forgot to clean it up. Done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File chrome/android/java_staging/res/values-v17/styles_staging.xml (right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/values-v17/styles_staging.xml:78: <style
name="EnhancedBookmarkDialogWhenLarge"
parent="Theme.AppCompat.Light.DialogWhenLarge">
On 2015/06/17 06:10:30, Kibeom Kim wrote:
> On 2015/06/17 04:42:22, newt wrote:
> > This is identical to EnhancedBookmarkDialogWhite. Why do you need this?
> 
> I guess it's because in this case we want the top action bar? Though name
> doesn't reflect it. Also, is there a reason not to use DialogWhenLarge style?
> Seems like it's identical just except android:statusBarColor and
> android:textColorLink.
> 
> If both are true, then I suggest something like:
> 
> <style name="DialogWhenLarge" parent="Theme.AppCompat.Light.DialogWhenLarge" >
>     <item name="android:windowBackground">@drawable/bg_white_dialog</item>
>     <item name="android:statusBarColor">@android:color/black</item>
>     <item name="android:textColorLink">@color/light_active_color</item>
>     <item name="colorPrimary">@color/light_active_color</item>
>     <item name="colorAccent">@color/light_active_color</item>
> </style>
> 
> <style name="DialogWhenLargeNoTitleActionBar" parent="DialogWhenLarge" >
>     <!-- Remove ActionBar -->
>     <item name="windowNoTitle">true</item>
>     <item name="windowActionBar">false</item>
> </style>

Reverted this change, because action bar cannot be added for free on tablet.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:22:
* The activity that enables the user to modify the title, url, parent folder of
a bookmark.
On 2015/06/17 04:42:22, newt wrote:
> "and parent folder"

Done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:25:
public static final String INTENT_BOOKMARK_ID =
"EnhancedBookmarkEditActivity.BookmarkId";
On 2015/06/17 04:42:23, newt wrote:
> document public constants

Done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:33:
private MenuItem mDeleteBtn;
On 2015/06/17 04:42:23, newt wrote:
> s/Btn/Button
> 
> Avoid abbrvtns

Done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:56:
if (mBookmarkId.equals(node.getId())) updateViewContent();
On 2015/06/17 06:10:30, Kibeom Kim wrote:
> Q: what happens if parent node's name is changed?

Then it's an old bug in detail page. I can fix it now.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:83:
BookmarkItem bookmarkItem =
mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);
On 2015/06/17 06:10:30, Kibeom Kim wrote:
> On 2015/06/17 04:42:23, newt wrote:
> > might this ever return null?
> 
> I don't know if it's possible for a bookmark to be deleted between
> EnhancedBookmarkEditActivity launching and here. Overly defensive null
checking
> seems considered harmful
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/KJdw-QiKdxY
> . How about landing as it is and monitoring crash reports?
> 
> Or, I'm not sure, but maybe this can happen when 1. user opens this activity
2.
> killed in background due to memory pressure. 3. user clears app data 4. Come
> back to this activity.

The framework will kill the app entirely if the user clicks "clear app data".

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:83:
BookmarkItem bookmarkItem =
mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);
On 2015/06/17 06:10:30, Kibeom Kim wrote:
> On 2015/06/17 04:42:23, newt wrote:
> > might this ever return null?
> 
> I don't know if it's possible for a bookmark to be deleted between
> EnhancedBookmarkEditActivity launching and here. Overly defensive null
checking
> seems considered harmful
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/KJdw-QiKdxY
> . How about landing as it is and monitoring crash reports?
> 
> Or, I'm not sure, but maybe this can happen when 1. user opens this activity
2.
> killed in background due to memory pressure. 3. user clears app data 4. Come
> back to this activity.

If the bookmark is deleted, the activity will be notified and it will call
finish(). I have seen several crashes where getbookmarkbyid() will cause
assertion failure because the bookmark does not exist anymore. However I don't
see any reason why the method would give me null ptr...

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:88:
mFolderTextView.setOnClickListener(new View.OnClickListener() {
On 2015/06/17 06:10:30, Kibeom Kim wrote:
> I guess we don't need to set listener on every update? Maybe in onCreate?

Done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:89:
@Override
On 2015/06/17 04:42:23, newt wrote:
> fix indentation

Done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:99:
mDeleteBtn = menu.add(0, 0, 0, "delete").setIcon(R.drawable.btn_trash);
On 2015/06/17 04:42:22, newt wrote:
> For the sake of accessibility users, use a localized string:
> 
>     menu.add(R.string.delete)...

Oops this is a mistake.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:106:
if (item.equals(mDeleteBtn)) {
On 2015/06/17 04:42:22, newt wrote:
> use == instead of equals(), since we only care about identity, not equality.

Done.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:121:
mEnhancedBookmarksModel.setBookmarkTitle(mBookmarkId, newTitle);
On 2015/06/17 04:42:23, newt wrote:
> why don't you validate newTitle and newUrl?

:P Done. However the error message will be handled by EmptyALertEditText.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/BookmarkItemView.java
(right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/ntp/BookmarkItemView.java:224:
mIsFolder ? R.string.contextmenu_edit_folder : R.string.edit_bookmark)
On 2015/06/17 06:10:30, Kibeom Kim wrote:
> I think we should also update the comment for IDS_EDIT_BOOKMARK string.
> Currently it's "Title of the "Edit Bookmark" form dialog"

Done.

[Kibeom Kim (inactive), 2015-06-17 21:18:25]

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:83:
BookmarkItem bookmarkItem =
mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);
On 2015/06/17 21:07:42, Ian Wen wrote:
> On 2015/06/17 06:10:30, Kibeom Kim wrote:
> > On 2015/06/17 04:42:23, newt wrote:
> > > might this ever return null?
> > 
> > I don't know if it's possible for a bookmark to be deleted between
> > EnhancedBookmarkEditActivity launching and here. Overly defensive null
> checking
> > seems considered harmful
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/KJdw-QiKdxY
> > . How about landing as it is and monitoring crash reports?
> > 
> > Or, I'm not sure, but maybe this can happen when 1. user opens this activity
> 2.
> > killed in background due to memory pressure. 3. user clears app data 4. Come
> > back to this activity.
> 
> If the bookmark is deleted, the activity will be notified and it will call
> finish(). I have seen several crashes where getbookmarkbyid() will cause
> assertion failure because the bookmark does not exist anymore. However I don't
> see any reason why the method would give me null ptr...

Since updateViewCount() is called in onCreate(), this can be null if bookmark is
deleted between activity launching and onCreate() here. Though, I don't know if
that can happen, or if there is any guarantee.

[newt (away), 2015-06-17 23:53:25]

I'm happy after a few small comments. Kibeom?

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File chrome/android/java_staging/res/layout/eb_edit.xml (right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/res/layout/eb_edit.xml:10:
android:layout_marginTop="8dp"
On 2015/06/17 21:07:42, Ian Wen wrote:
> On 2015/06/17 04:42:22, newt wrote:
> > why not margin bottom?
> 
> Because on tablet the size of the dialog is fixed and setting margin won't
> affect its size. On phone 99% of the devices are large enough to hold the
simple
> content.

99%? This doesn't sound very scientific ;) What about in multi-window mode, or
in landscape on small devices? In any case, I'd add a marginBottom for symmetry.

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:27:
/** The name of the extra in the intent to pass the bookmark id. */
"The intent extra specifying the ID of the bookmark to be edited."

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:108:
mDeleteButton = menu.add(0, Menu.NONE, 0,
menu.add(R.string.enhanced_bookmark_action_bar_delete)

[Kibeom Kim (inactive), 2015-06-18 00:25:37]

Seems good overall to me. Thanks!

I just have one concern regarding back button flow and validation.

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/1/chrome/android/java_staging...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:83:
BookmarkItem bookmarkItem =
mEnhancedBookmarksModel.getBookmarkById(mBookmarkId);
On 2015/06/17 21:18:25, Kibeom Kim wrote:
> On 2015/06/17 21:07:42, Ian Wen wrote:
> > On 2015/06/17 06:10:30, Kibeom Kim wrote:
> > > On 2015/06/17 04:42:23, newt wrote:
> > > > might this ever return null?
> > > 
> > > I don't know if it's possible for a bookmark to be deleted between
> > > EnhancedBookmarkEditActivity launching and here. Overly defensive null
> > checking
> > > seems considered harmful
> > >
> >
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/KJdw-QiKdxY
> > > . How about landing as it is and monitoring crash reports?
> > > 
> > > Or, I'm not sure, but maybe this can happen when 1. user opens this
activity
> > 2.
> > > killed in background due to memory pressure. 3. user clears app data 4.
Come
> > > back to this activity.
> > 
> > If the bookmark is deleted, the activity will be notified and it will call
> > finish(). I have seen several crashes where getbookmarkbyid() will cause
> > assertion failure because the bookmark does not exist anymore. However I
don't
> > see any reason why the method would give me null ptr...
> 
> Since updateViewCount() is called in onCreate(), this can be null if bookmark
is
> deleted between activity launching and onCreate() here. Though, I don't know
if
> that can happen, or if there is any guarantee.

Discussed offline.

Ian pointed out that EnhancedBookmarksModel#getBookmarkById doesn't return null,
but it crashes on passing an invalid ID. I don't have confidence that assuming
mBookmarkId valid is safe for the flow mentioned above, but anyways we have the
same problem across our codebase overall, and we're indeed getting crashes.
https://code.google.com/p/chromium/issues/detail?id=433431 I think we should
address this issue separately so let's leave as it is. I'm considering making
EnhancedBookmarksModel.getBookmarkById() actually return null so that we know
what places are causing problems.

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
File chrome/android/java_staging/res/layout/eb_edit.xml (right):

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/res/layout/eb_edit.xml:17:
android:minHeight="?attr/actionBarSize" />
Btw, do you know if we're doing shadow for toolbar in general? The new mock
doesn't clearly show it, maybe we want to clarify with Chris.

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:140:
}
Q1: How are you planning to structure invalid url warning?

Q2: Not letting to escape when a field is empty seems too invasive for user
action. ianwen@, newt@: any thoughts?

[newt (away), 2015-06-18 00:31:26]

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:140:
}
On 2015/06/18 00:25:37, Kibeom Kim wrote:
> Q1: How are you planning to structure invalid url warning?
> 
> Q2: Not letting to escape when a field is empty seems too invasive for user
> action. ianwen@, newt@: any thoughts?

If we had a cancel or "x" button, the user could press it to escape this dialog,
even when the current input URL or title is invalid. Without adding such a
button, I don't have any ideas. Maybe ask the designer?

[Ian Wen, 2015-06-18 01:19:34]

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
File chrome/android/java_staging/res/layout/eb_edit.xml (right):

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/res/layout/eb_edit.xml:17:
android:minHeight="?attr/actionBarSize" />
On 2015/06/18 00:25:37, Kibeom Kim wrote:
> Btw, do you know if we're doing shadow for toolbar in general? The new mock
> doesn't clearly show it, maybe we want to clarify with Chris.

Hmm I can add a shadow here. (personally it looks better to me without the
shadow, but it seems having a shadow below the snackbar is a standard.)

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:27:
/** The name of the extra in the intent to pass the bookmark id. */
On 2015/06/17 23:53:25, newt wrote:
> "The intent extra specifying the ID of the bookmark to be edited."

Done.

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:108:
mDeleteButton = menu.add(0, Menu.NONE, 0,
On 2015/06/17 23:53:25, newt wrote:
> menu.add(R.string.enhanced_bookmark_action_bar_delete)

Done.

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:140:
}
On 2015/06/18 00:25:37, Kibeom Kim wrote:
> Q1: How are you planning to structure invalid url warning?
> 
> Q2: Not letting to escape when a field is empty seems too invasive for user
> action. ianwen@, newt@: any thoughts?

1. https://codereview.chromium.org/1187013002/.

2. We might discuss with Chris (or maybe in the next UI review). My personal
feeling is that simplicity is the top priority for me and having one return
button looks best. For users who insist on pressing back button with an empty
field, he would have noticed the red warning on the edittext.

[Kibeom Kim (inactive), 2015-06-18 02:28:13]

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:140:
}
On 2015/06/18 01:19:34, Ian Wen wrote:
> On 2015/06/18 00:25:37, Kibeom Kim wrote:
> > Q1: How are you planning to structure invalid url warning?
> > 
> > Q2: Not letting to escape when a field is empty seems too invasive for user
> > action. ianwen@, newt@: any thoughts?
> 
> 1. https://codereview.chromium.org/1187013002/.

(I thought we had a proper URL validation but It seems it wasn't properly
implemented before :/ )

1. If title or URL is invalid, then we shouldn't save at #134-135. We didn't
save in the previous code by "if (urlOrTitleInvalid) return false;"
2. We should distinguish empty and invalid URL error and show different error
messages. In the old code, we forcefully cleared the URL EditText and in the new
code, we kept showing the invalid URL while the error message is "URL required".
Both are not ideal.

The second one is not a regression so please feel free to file a bug instead.
(but if so, at least keep the old behavior of clearing URL EditText)

[Ian Wen, 2015-06-18 17:27:13]

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
File
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1182083005/diff/20001/chrome/android/java_sta...
chrome/android/java_staging/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:140:
}
On 2015/06/18 02:28:13, Kibeom Kim wrote:
> On 2015/06/18 01:19:34, Ian Wen wrote:
> > On 2015/06/18 00:25:37, Kibeom Kim wrote:
> > > Q1: How are you planning to structure invalid url warning?
> > > 
> > > Q2: Not letting to escape when a field is empty seems too invasive for
user
> > > action. ianwen@, newt@: any thoughts?
> > 
> > 1. https://codereview.chromium.org/1187013002/.
> 
> (I thought we had a proper URL validation but It seems it wasn't properly
> implemented before :/ )
> 
> 1. If title or URL is invalid, then we shouldn't save at #134-135. We didn't
> save in the previous code by "if (urlOrTitleInvalid) return false;"
> 2. We should distinguish empty and invalid URL error and show different error
> messages. In the old code, we forcefully cleared the URL EditText and in the
new
> code, we kept showing the invalid URL while the error message is "URL
required".
> Both are not ideal.
> 
> The second one is not a regression so please feel free to file a bug instead.
> (but if so, at least keep the old behavior of clearing URL EditText)

Done.

[Ian Wen, 2015-06-19 00:18:10]

In the most recent patchset, EmptyAlertEditText is adopted. Also description
code in ebmodel, bridges are removed as well. ptal.

[newt (away), 2015-06-19 00:39:25]

lgtm

[Kibeom Kim (inactive), 2015-06-19 00:47:38]

lgtm

[Ian Wen, 2015-06-19 16:31:53]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2015-06-19 16:32:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1182083005/80001

[commit-bot: I haz the power, 2015-06-19 17:39:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-19 17:40:34]

Patchset 5 (id:??) landed as
https://crrev.com/474f656d3f477d51f49b56bb17125dccd7d035cf
Cr-Commit-Position: refs/heads/master@{#335283}

[Kibeom Kim (inactive), 2015-06-22 21:24:22]

I just noticed that SW keyboard is persistent even after being dismissed.