Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1230313009:
[Android] Make enhanced bookmark editing UI more straightforward. (Closed)
|
Created:
5 years, 5 months ago by Kibeom Kim (inactive) Modified:
5 years, 4 months ago CC:
chromium-reviews, ianwen+watch_chromium.org, newt (away) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Make enhanced bookmark editing UI more straightforward.
- We didn't save when a dialog is dismissed by tapping outside on
tablet, which can be also confusing as there is no save UI concept.
So save onPause always.
- User should know when we won't save what user typed if it fails
validation. But blocking dialog exit is too invasive, and sometimes
we don't have a control (tapping outside on tablet.) So show warning
as user types.
BUG=510253
Committed: https://crrev.com/5886fc8beb1ae5714f25a8bb1c76a42bfe88272c
Cr-Commit-Position: refs/heads/master@{#342719}
Patch Set 1 #Patch Set 2 : #
Total comments: 13
Patch Set 3 : addressed comments #
Total comments: 16
Patch Set 4 : removed less important changes for M45 merge #
Total comments: 4
Patch Set 5 : Also update EnhancedbookmarkAddEditFolderActivity #
Total comments: 4
Patch Set 6 : updated margins #
Total comments: 8
Patch Set 7 : patchset 6 comments #Patch Set 8 : reverted mojo change #
Messages
Total messages: 23 (4 generated)
kkimlabs@chromium.org changed reviewers: + ianwen@chromium.org, tedchoc@chromium.org
https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:38: <!-- A trick to avoid focusing the below EditText initially. For some reason, setting Use SOFT_INPUT_STATE_HIDDEN instead. You can also configure it in xml. https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:51: android:layout_marginBottom="10dp" > When layouting edittexts, paddings are in general preferred, because it increases the touch area and make it more convenient to edit. That was the reason why I did not choose to use margin in the first place. https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:91: mTitleEditText.validate(); Isn't this already done in EmptyAlertEdittext? https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:133: if (!bookmarkItem.getTitle().equals(mTitleEditText.getTrimmedText())) { updateViewContent() is only called twice: once by onCreate, once by onBookmarkNodeChange(). If it is not a frequent call, I don't see the value of adding additional if checks here. https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:141: mUrlEditText.setText(bookmarkItem.getUrl()); If textview's text is not correct, while it's fix'd-up version is correct and is the same as bookmarkItem.getUrl(). Then it will not be updated? https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:169: protected void onDestroy() { Don't save data in onDestroy(). I would prefer using onPause() instead. Per android doc: "Note: do not count on this method being called as a place for saving data! For example, if an activity is editing data in a content provider, those edits should be committed in either onPause() or onSaveInstanceState(Bundle), not here. This method is usually implemented to free resources like threads that are associated with an activity, so that a destroyed activity does not leave such things around while the rest of its application is still running. There are situations where the system will simply kill the activity's hosting process without calling this method (or any others) in it, so it should not be used to do things that are intended to remain around after the process goes away."
https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:38: <!-- A trick to avoid focusing the below EditText initially. For some reason, setting On 2015/07/20 18:41:57, Ian Wen wrote: > Use SOFT_INPUT_STATE_HIDDEN instead. You can also configure it in xml. Yeah keyboard hiding is already configured in the xml. This removes focus. (it can be focused without keyboard) https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:51: android:layout_marginBottom="10dp" > On 2015/07/20 18:41:57, Ian Wen wrote: > When layouting edittexts, paddings are in general preferred, because it > increases the touch area and make it more convenient to edit. That was the > reason why I did not choose to use margin in the first place. Actually, that's the reason I used margin because I think we shouldn't enlarge touch area unnecessarily. Do you know if there is a design guideline around this? But anyways, I believe that TextInputLayout doesn't get touch-focus itself. https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:91: mTitleEditText.validate(); On 2015/07/20 18:41:58, Ian Wen wrote: > Isn't this already done in EmptyAlertEdittext? Yeah it removes it, but doesn't add a warning. I was thinking putting this logic in EmptyAlertEditText (maybe as an option). wdyt? https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:133: if (!bookmarkItem.getTitle().equals(mTitleEditText.getTrimmedText())) { On 2015/07/20 18:41:58, Ian Wen wrote: > updateViewContent() is only called twice: once by onCreate, once by > onBookmarkNodeChange(). If it is not a frequent call, I don't see the value of > adding additional if checks here. Text cursor position is changed as a side-effect of setting text, it was to avoid that. https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:141: mUrlEditText.setText(bookmarkItem.getUrl()); On 2015/07/20 18:41:58, Ian Wen wrote: > If textview's text is not correct, while it's fix'd-up version is correct and is > the same as bookmarkItem.getUrl(). Then it will not be updated? Yeah I agree that fixupUrl doesn't fit here. Done. https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:169: protected void onDestroy() { On 2015/07/20 18:41:58, Ian Wen wrote: > Don't save data in onDestroy(). I would prefer using onPause() instead. > > Per android doc: > "Note: do not count on this method being called as a place for saving data! For > example, if an activity is editing data in a content provider, those edits > should be committed in either onPause() or onSaveInstanceState(Bundle), not > here. This method is usually implemented to free resources like threads that are > associated with an activity, so that a destroyed activity does not leave such > things around while the rest of its application is still running. There are > situations where the system will simply kill the activity's hosting process > without calling this method (or any others) in it, so it should not be used to > do things that are intended to remain around after the process goes away." Done.
This CL gives me the impression that the changes are created in an ad-hoc way. Has there been a discussion about the final UI behavior? Was the decision made somewhere? In the referenced bug I was the last person to respond and it seems nobody else commented. We had better have a conclusion first then start drafting a CL. :) https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:40: in this context. --> I don't see it is a problem to let title edit text have focus (the keyboard is already not shown)... Yet if you insist, we could simply make the shadow view (#19) to be focusable. Also android:focusableInTouch alone should do the job. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:51: android:layout_marginBottom="10dp" > Is there a clear advantage of using margin instead of padding? Are there any bugs filed for this issue? If not I would prefer keeping things the way it was... https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:169: protected void onPause() { If seems if the user types a wrong url and click back to save it, then in below we will do nothing. Then later when the user checks back he will notice his new url was lost. You need to somehow notify the user that he typed something wrong. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java:69: public boolean isValid() { 1. isValid() "checks whether the content is empty". Then it should be named as isEmpty() instead. 2. This method returns the same thing as validate(), and I wonder why can't we use validate() in the activity? Is it a bad thing to tell the user that the text is empty and it shouldn't be?
This CL gives me the impression that the changes are created in an ad-hoc way. Has there been a discussion about the final UI behavior? Was the decision made somewhere? In the referenced bug I was the last person to respond and it seems nobody else has commented since then. We had better have a conclusion first then start drafting a CL. :)
https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:44: android:focusable="true" Can you set these values to false on the textviews? Then when it is attached to the window or something, we could set them to true to allow them to be focused? Does this affect accessibility focus traversal? https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:133: if (!bookmarkItem.getTitle().equals(mTitleEditText.getTrimmedText())) { I would use TextUtils.equals for all of these. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:137: if (!folderTitle.equals(mFolderTextView.getText().toString())) { I wonder if you need toString if you use TextUtils. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java:66: *Checks whether the content is empty. Note that this doesn't update an error message. need a blank space after *
https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:40: in this context. --> On 2015/07/21 19:59:05, Ian Wen wrote: > I don't see it is a problem to let title edit text have focus (the keyboard is > already not shown)... Yet if you insist, we could simply make the shadow view > (#19) to be focusable. Also android:focusableInTouch alone should do the job. not critical for M45 removed. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:44: android:focusable="true" On 2015/07/21 20:21:08, Ted C (OOO till 8.21) wrote: > Can you set these values to false on the textviews? Then when it is attached to > the window or something, we could set them to true to allow them to be focused? > > Does this affect accessibility focus traversal? not critical for M45 removed. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:51: android:layout_marginBottom="10dp" > On 2015/07/21 19:59:05, Ian Wen wrote: > Is there a clear advantage of using margin instead of padding? Are there any > bugs filed for this issue? If not I would prefer keeping things the way it > was... discussed offline https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:133: if (!bookmarkItem.getTitle().equals(mTitleEditText.getTrimmedText())) { On 2015/07/21 20:21:08, Ted C (OOO till 8.21) wrote: > I would use TextUtils.equals for all of these. Done. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:137: if (!folderTitle.equals(mFolderTextView.getText().toString())) { On 2015/07/21 20:21:08, Ted C (OOO till 8.21) wrote: > I wonder if you need toString if you use TextUtils. Done. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:169: protected void onPause() { On 2015/07/21 19:59:05, Ian Wen wrote: > If seems if the user types a wrong url and click back to save it, then in below > we will do nothing. Then later when the user checks back he will notice his new > url was lost. > > You need to somehow notify the user that he typed something wrong. discussed offline https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java (right): https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java:66: *Checks whether the content is empty. Note that this doesn't update an error message. On 2015/07/21 20:21:08, Ted C (OOO till 8.21) wrote: > need a blank space after * Done. https://codereview.chromium.org/1230313009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java:69: public boolean isValid() { On 2015/07/21 19:59:05, Ian Wen wrote: > 1. isValid() "checks whether the content is empty". Then it should be named as > isEmpty() instead. > > 2. This method returns the same thing as validate(), and I wonder why can't we > use validate() in the activity? Is it a bad thing to tell the user that the text > is empty and it shouldn't be? in the new code, we call validate() on every text change so calling validate() in onPause is redundant. but I guess it shouldn't matter. Removed.
This CL looks good to me in general. Hold back for one question. https://codereview.chromium.org/1230313009/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:69: updateViewContent(); Why adding this? Aren't we safe having the checks above? https://codereview.chromium.org/1230313009/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:91: mTitleEditText.validate(); EBAddEditFolderActivity and EBEditActivity are the only two clients that use EmptyAlertEdittext. You can just modify it so that it does this kind of realtime checking. Though it's up to you if you want to do that in M45 or not.
Also updated EnhancedBookmarkAddEditFolderActivity https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:91: mTitleEditText.validate(); On 2015/07/20 20:04:48, Kibeom Kim wrote: > On 2015/07/20 18:41:58, Ian Wen wrote: > > Isn't this already done in EmptyAlertEdittext? > > Yeah it removes it, but doesn't add a warning. I was thinking putting this logic > in EmptyAlertEditText (maybe as an option). wdyt? Done. https://codereview.chromium.org/1230313009/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:69: updateViewContent(); On 2015/08/05 00:44:27, Ian Wen wrote: > Why adding this? Aren't we safe having the checks above? This is not called on extensive change. https://codereview.chromium.org/1230313009/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:91: mTitleEditText.validate(); On 2015/08/05 00:44:27, Ian Wen wrote: > EBAddEditFolderActivity and EBEditActivity are the only two clients that use > EmptyAlertEdittext. You can just modify it so that it does this kind of realtime > checking. Though it's up to you if you want to do that in M45 or not. Done.
kkimlabs@chromium.org changed reviewers: + newt@chromium.org - tedchoc@chromium.org
-tedchoc@ +newt@: ptal
I downloaded the patch and played with it a bit. It looks pretty neat. https://codereview.chromium.org/1230313009/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:29: <LinearLayout The added spacings are too much. In mocks this linear layout's height is 208 dp, in the new view the total height becomes 242dp. https://codereview.chromium.org/1230313009/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:140: protected void onPause() { Let's show a toast notifying whether a save operation has been conducted or not (also recommended by cleer@ in the bug). Later I will update the toast to use actionless snackbar (not in M45).
newt@: ptal https://codereview.chromium.org/1230313009/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/eb_edit.xml:29: <LinearLayout On 2015/08/07 17:22:53, Ian Wen wrote: > The added spacings are too much. In mocks this linear layout's height is 208 dp, > in the new view the total height becomes 242dp. still different but confirmed with cleer@ at the bug. https://codereview.chromium.org/1230313009/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:140: protected void onPause() { On 2015/08/07 17:22:53, Ian Wen wrote: > Let's show a toast notifying whether a save operation has been conducted or not > (also recommended by cleer@ in the bug). Later I will update the toast to use > actionless snackbar (not in M45). I'll do as a follow up CL since I can't merge string to M45.
https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/eb_edit.xml:60: android:textAppearance="?android:attr/textAppearanceSmall" Beware that textAppearanceSmall is different on L vs pre-L devices. Using TextAppearance.AppCompat.Small will get you a consistent look, on the other hand. Same for textAppearanceMedium below. https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkAddEditFolderActivity.java (right): https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkAddEditFolderActivity.java:217: protected void onPause() { onStop() is a more normal place to save program state. It's guaranteed to be called when the activity goes away, and won't be called as many times as onPause() (which happens when a window appears above the activity, or when the user slides down the notification bar, etc). Use onStop() instead? https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:151: protected void onPause() { onStop()? https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java (right): https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java:70: * @return Whether the content is valid. Please define "valid", or change this to "empty". isEmpty() seems like a clearer and simpler name, btw.
https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/re... File chrome/android/java/res/layout/eb_edit.xml (right): https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/re... chrome/android/java/res/layout/eb_edit.xml:60: android:textAppearance="?android:attr/textAppearanceSmall" On 2015/08/10 20:15:43, newt wrote: > Beware that textAppearanceSmall is different on L vs pre-L devices. Using > TextAppearance.AppCompat.Small will get you a consistent look, on the other > hand. > > Same for textAppearanceMedium below. Done. https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkAddEditFolderActivity.java (right): https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkAddEditFolderActivity.java:217: protected void onPause() { On 2015/08/10 20:15:43, newt wrote: > onStop() is a more normal place to save program state. It's guaranteed to be > called when the activity goes away, and won't be called as many times as > onPause() (which happens when a window appears above the activity, or when the > user slides down the notification bar, etc). Use onStop() instead? Done. https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java (right): https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:151: protected void onPause() { On 2015/08/10 20:15:43, newt wrote: > onStop()? Done. https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java (right): https://codereview.chromium.org/1230313009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/widget/EmptyAlertEditText.java:70: * @return Whether the content is valid. On 2015/08/10 20:15:43, newt wrote: > Please define "valid", or change this to "empty". isEmpty() seems like a clearer > and simpler name, btw. Done.
revert changes in mojo, then lgtm
The CQ bit was checked by kkimlabs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org Link to the patchset: https://codereview.chromium.org/1230313009/#ps140001 (title: "reverted mojo change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1230313009/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1230313009/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/5886fc8beb1ae5714f25a8bb1c76a42bfe88272c Cr-Commit-Position: refs/heads/master@{#342719}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1278963004/ by kkimlabs@chromium.org. The reason for reverting is: http://crbug.com/519368. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
