Adding a dialog on click of "add to homescreen" menu item to edit the title.

Adding a dialog on click of "add to homescreen" menu item to edit the title.

BUG=289743
R=dfalcantara@chromium.org, klobag@chromium.org, yfriedman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222950

[klobag.chromium, 2013-09-13 00:25:29]

Is there another CL for the dialog?

[Ramya, 2013-09-13 00:27:41]

Yes, https://gerrit-int.chromium.org/#/c/44134/

[gone, 2013-09-13 00:31:52]

https://chromiumcodereview.appspot.com/24063004/diff/5001/chrome/android/java...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://chromiumcodereview.appspot.com/24063004/diff/5001/chrome/android/java...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:77:
Intent homeIntent = new Intent(Intent.ACTION_MAIN);
nit: Comment that this sends the user to the home screen.

https://chromiumcodereview.appspot.com/24063004/diff/23001/chrome/android/jav...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://chromiumcodereview.appspot.com/24063004/diff/23001/chrome/android/jav...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:77:
Intent homeIntent = new Intent(Intent.ACTION_MAIN);
Add comment here and in the javadoc saying it sends you to the home screen.

https://chromiumcodereview.appspot.com/24063004/diff/23001/chrome/android/jav...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://chromiumcodereview.appspot.com/24063004/diff/23001/chrome/android/jav...
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java:85:
addShortcutToURL(WEBAPP_HTML, "");
Can you add a unit test to see if your override works correctly?

https://chromiumcodereview.appspot.com/24063004/diff/23001/chrome/browser/and...
File chrome/browser/android/shortcut_helper.cc (right):

https://chromiumcodereview.appspot.com/24063004/diff/23001/chrome/browser/and...
chrome/browser/android/shortcut_helper.cc:34: if (title.length() != 0)
nit: > 0

[klobag.chromium, 2013-09-13 00:41:14]

lgtm

Please address Dan's comments.

[Ramya, 2013-09-13 01:04:34]

https://codereview.chromium.org/24063004/diff/23001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/24063004/diff/23001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:77:
Intent homeIntent = new Intent(Intent.ACTION_MAIN);
On 2013/09/13 00:31:53, dfalcantara wrote:
> Add comment here and in the javadoc saying it sends you to the home screen.

Done.

https://codereview.chromium.org/24063004/diff/23001/chrome/android/javatests/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://codereview.chromium.org/24063004/diff/23001/chrome/android/javatests/...
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java:85:
addShortcutToURL(WEBAPP_HTML, "");
On 2013/09/13 00:31:53, dfalcantara wrote:
> Can you add a unit test to see if your override works correctly?

Done.

https://codereview.chromium.org/24063004/diff/23001/chrome/browser/android/sh...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/24063004/diff/23001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:34: if (title.length() != 0)
On 2013/09/13 00:31:53, dfalcantara wrote:
> nit: > 0

Done.

[gone, 2013-09-13 01:07:07]

lgtm

[Yaron, 2013-09-13 01:13:16]

lgtm with changes listed

https://codereview.chromium.org/24063004/diff/34001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/24063004/diff/34001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:39: *
@param tab Tab to create a shortcut for.
Add @param

https://codereview.chromium.org/24063004/diff/34001/chrome/android/javatests/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://codereview.chromium.org/24063004/diff/34001/chrome/android/javatests/...
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java:136:
addShortcutToURL(WEBAPP_HTML, EDITED_WEBAPP_TITLE );
nit: extra ws

https://codereview.chromium.org/24063004/diff/34001/chrome/browser/android/sh...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/24063004/diff/34001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:30: string16 title)
const string16& to avoid copies

https://codereview.chromium.org/24063004/diff/34001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:116: string16 title) {
const string16&

[Ramya, 2013-09-13 01:19:46]

https://codereview.chromium.org/24063004/diff/34001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/24063004/diff/34001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:39: *
@param tab Tab to create a shortcut for.
On 2013/09/13 01:13:16, Yaron wrote:
> Add @param

Done.

https://codereview.chromium.org/24063004/diff/34001/chrome/android/javatests/...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java
(right):

https://codereview.chromium.org/24063004/diff/34001/chrome/android/javatests/...
chrome/android/javatests/src/org/chromium/chrome/browser/ShortcutHelperTest.java:136:
addShortcutToURL(WEBAPP_HTML, EDITED_WEBAPP_TITLE );
On 2013/09/13 01:13:16, Yaron wrote:
> nit: extra ws

Done.

https://codereview.chromium.org/24063004/diff/34001/chrome/browser/android/sh...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/24063004/diff/34001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:30: string16 title)
On 2013/09/13 01:13:16, Yaron wrote:
> const string16& to avoid copies

Done.

https://codereview.chromium.org/24063004/diff/34001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:30: string16 title)
On 2013/09/13 01:13:16, Yaron wrote:
> const string16& to avoid copies

Done.

https://codereview.chromium.org/24063004/diff/34001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:116: string16 title) {
On 2013/09/13 01:13:16, Yaron wrote:
> const string16&

Done.

https://codereview.chromium.org/24063004/diff/34001/chrome/browser/android/sh...
chrome/browser/android/shortcut_helper.cc:116: string16 title) {
On 2013/09/13 01:13:16, Yaron wrote:
> const string16&

Done.

[Ramya, 2013-09-13 01:48:11]

Committed patchset #7 manually as r222950 (presubmit successful).