Refactor ShortcutHelper and merge in BookmarkUtils

Refactor ShortcutHelper and merge in BookmarkUtils

This CL reworks the terminology used by classes which are
used to add shortcuts to the homescreen. What it does
exactly is:

(1) Splits off all functions in ShortcutHelper which are
only used by Add to Homescreen dialog into their own file
called AddToHomescreenDialogHelper (both Java and C++
componenets)

(2) Merges BookmarkUtils with the one remaining function
from ShortcutHelper into a class called ShortcutHelper.
BookmarkUtils is a bad name since these are not actually
Chrome bookmarks and moreover, there is test utility class
called BookmarkTestUtils which works on Chrome bookmarks.

(3) Changes ShortcutDataFetcher to
AddToHomescreenDialogFetcher to match the change in (1).

(4) Fixes all usage of functions to be correct with the new
names.

BUG=508627
Committed: https://crrev.com/870920edb6dd146b86d84d99cbeee1c47c76ce53
Cr-Commit-Position: refs/heads/master@{#344595}

[Lalit Maganti, 2015-08-13 12:26:18]

lalitm@google.com changed reviewers:
+ dfalcantara@chromium.org, mlamouri@chromium.org

[Lalit Maganti, 2015-08-13 12:26:19]

The patch compiles but I still need to split the test files as well for it to be
complete.

[Lalit Maganti, 2015-08-13 13:08:04]

Dan and Mounir: I've moved the test case over too so this should be ready to
look at now.

[mlamouri (slow - plz ping), 2015-08-13 13:18:26]

Could you double-check that the renamings are fine?

lgtm with that and comments below applied.

https://codereview.chromium.org/1288903002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java
(right):

https://codereview.chromium.org/1288903002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:78:
final AddToHomescreenHelper shortcutHelper =
Maybe you should rename the variable?

https://codereview.chromium.org/1288903002/diff/20001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/1288903002/diff/20001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:253:
ShortcutHelper::RegisterShortcutHelper},
nit: why did you need to add a line break here?

https://codereview.chromium.org/1288903002/diff/20001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.cc (right):

https://codereview.chromium.org/1288903002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.cc:22: #include "url/gurl.h"
nit: you might want to remove some includes.

https://codereview.chromium.org/1288903002/diff/20001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.h (right):

https://codereview.chromium.org/1288903002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:16: class ShortcutHelper {
nit: could you make the class final?

https://codereview.chromium.org/1288903002/diff/20001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:26: ShortcutHelper();
Do you really need the ctor? Maybe mark it as = delete;

[gone, 2015-08-13 20:38:23]

Haven't reviewed yet, but your commit message doesn't make sense and is probably
the result of not knowing the history of how adding anything to the home screen
originally worked.  Adding a shortcut is the same thing as adding something to
the home screen.  The only difference is that one kind opens in a WebappActivity
and the other kind opens up the regular browser activity.  Both require
downloading some icon representative of the page, which used to be just a
favicon or some meta-tag defined icon, which is why the ShortcutDataFetcher
dealth with it.

This code got convoluted with the introduction of manifests and banners because
they introduced yet another source for icons and other web app data.

[gone, 2015-08-13 20:54:28]

Yeah, I'm confused by your choice of split since it seems that you believe that
there are two completely distinct pathways based on your commit message:
"AddToHomescreenHelper which contains the codepath for adding webapps to the
homescreen."

Can you confirm that adding a non-webapp shortcut via the menu item (e.g. for
something like http://reddit.com or http://erfworld.com) will still go through
the renamed AddToHomescreenHelper* classes?

https://chromiumcodereview.appspot.com/1288903002/diff/40001/chrome/browser/a...
File chrome/browser/android/webapps/add_to_homescreen_helper.h (right):

https://chromiumcodereview.appspot.com/1288903002/diff/40001/chrome/browser/a...
chrome/browser/android/webapps/add_to_homescreen_helper.h:25: // ShortcutHelper
is the C++ counterpart of org.chromium.chrome.browser's
you didn't change any of the comments to change the ShortcutHelper references

[Lalit Maganti, 2015-08-13 21:00:05]

On 2015/08/13 20:54:28, dfalcantara wrote:
> Yeah, I'm confused by your choice of split since it seems that you believe
that
> there are two completely distinct pathways based on your commit message:
> "AddToHomescreenHelper which contains the codepath for adding webapps to the
> homescreen."
> 
> Can you confirm that adding a non-webapp shortcut via the menu item (e.g. for
> something like http://reddit.com or http://erfworld.com) will still go through
> the renamed AddToHomescreenHelper* classes?
> 
>
https://chromiumcodereview.appspot.com/1288903002/diff/40001/chrome/browser/a...
> File chrome/browser/android/webapps/add_to_homescreen_helper.h (right):
> 
>
https://chromiumcodereview.appspot.com/1288903002/diff/40001/chrome/browser/a...
> chrome/browser/android/webapps/add_to_homescreen_helper.h:25: //
ShortcutHelper
> is the C++ counterpart of org.chromium.chrome.browser's
> you didn't change any of the comments to change the ShortcutHelper references

I probably haven't explained it quite well but what I'm trying to say is that
the app banner stuff is using the current shortcut helper class to do the actual
adding of the shortcut to homescreen (i.e. the function I've left in the
ShortcutHelper class). The new AddToHomescreen class contains all the functions
which are used when "Add To Homescreen" is selected from the menu item (i.e. it
no longer contains functions which are also used by AppBanner). Right now there
is only one common function so it looks OK but when splashscreen code is added
there will end up being several common functions which would all go in the new
ShortcutHelper class.

[Lalit Maganti, 2015-08-13 21:06:20]

On 2015/08/13 21:00:05, Lalit Maganti wrote:
> On 2015/08/13 20:54:28, dfalcantara wrote:
> > Yeah, I'm confused by your choice of split since it seems that you believe
> that
> > there are two completely distinct pathways based on your commit message:
> > "AddToHomescreenHelper which contains the codepath for adding webapps to the
> > homescreen."
> > 
> > Can you confirm that adding a non-webapp shortcut via the menu item (e.g.
for
> > something like http://reddit.com or http://erfworld.com) will still go
through
> > the renamed AddToHomescreenHelper* classes?
> > 
> >
>
https://chromiumcodereview.appspot.com/1288903002/diff/40001/chrome/browser/a...
> > File chrome/browser/android/webapps/add_to_homescreen_helper.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1288903002/diff/40001/chrome/browser/a...
> > chrome/browser/android/webapps/add_to_homescreen_helper.h:25: //
> ShortcutHelper
> > is the C++ counterpart of org.chromium.chrome.browser's
> > you didn't change any of the comments to change the ShortcutHelper
references
> 
> I probably haven't explained it quite well but what I'm trying to say is that
> the app banner stuff is using the current shortcut helper class to do the
actual
> adding of the shortcut to homescreen (i.e. the function I've left in the
> ShortcutHelper class). The new AddToHomescreen class contains all the
functions
> which are used when "Add To Homescreen" is selected from the menu item (i.e.
it
> no longer contains functions which are also used by AppBanner). Right now
there
> is only one common function so it looks OK but when splashscreen code is added
> there will end up being several common functions which would all go in the new
> ShortcutHelper class.

So to answer your question directly, yes that would still go through the
AddToHomescreen classes.

[gone, 2015-08-13 21:43:15]

I get the gist of what you're doing, but you need to update the commit message
to reflect what's really going on.

1) Is there a implementation doc somewhere that describes how the code reviews
you're sending are all interconnected?  I'm flying relatively blind on why
things are happening or moving around.

2) You need to be extremely careful when copy-pasting files and splitting
classes apart.  Most of the comments I saw don't make sense anymore and need to
be updated.

3) AddToHomescreeenHelper and ShortcutHelper sound like they should be doing the
same thing, name-wise, but I can't give you a better one to use without knowing
what's coming down the pipe.

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java
(right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:24:
* logic to ShortcutHelper.
Update all comments to reflect the split and what they do now.  Also refer to
the right classes.  May as well change valid references to "shortcut helper" to
"ShortcutHelper" while you're in here.

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java
(right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java:17:
* This is a helper class to create shortcuts on the Android home screen.
Change the comment to better describe what this thing does post-split.

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java:84:
* @param userRequestedTitle Updated title for the shortcut.
nit: Not sure what an "Updated title for the shortcut" means without any context
from places where this function is called.

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelperTest.java
(right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelperTest.java:27:
* Tests org.chromium.chrome.browser.webapps.AddToHomescreenHelper and it's C++
counterpart.
nit: it's -> its

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.h (right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:15: // from there via a JNI
(Initialize) call and MUST BE DESTROYED via Destroy().
Update the class description.  There's no Destroy() call anymore (e.g.)

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:5: #ifndef
CHROME_BROWSER_ANDROID_WEBAPPS_ADD_TO_HOMESCREEN_DATA_FETCHER_H_
Does it make sense to move these to the webapps/ directory, given that these
aren't used _just_ for webapps?

[Lalit Maganti, 2015-08-13 21:50:00]

On 2015/08/13 21:43:15, dfalcantara wrote:
> I get the gist of what you're doing, but you need to update the commit message
> to reflect what's really going on.
> 
> 1) Is there a implementation doc somewhere that describes how the code reviews
> you're sending are all interconnected?  I'm flying relatively blind on why
> things are happening or moving around.
> 
> 2) You need to be extremely careful when copy-pasting files and splitting
> classes apart.  Most of the comments I saw don't make sense anymore and need
to
> be updated.
> 
> 3) AddToHomescreeenHelper and ShortcutHelper sound like they should be doing
the
> same thing, name-wise, but I can't give you a better one to use without
knowing
> what's coming down the pipe.
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java
> (right):
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:1:
> // Copyright 2014 The Chromium Authors. All rights reserved.
> nit: 2015
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:24:
> * logic to ShortcutHelper.
> Update all comments to reflect the split and what they do now.  Also refer to
> the right classes.  May as well change valid references to "shortcut helper"
to
> "ShortcutHelper" while you're in here.
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java
> (right):
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java:17:
> * This is a helper class to create shortcuts on the Android home screen.
> Change the comment to better describe what this thing does post-split.
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java:84:
> * @param userRequestedTitle Updated title for the shortcut.
> nit: Not sure what an "Updated title for the shortcut" means without any
context
> from places where this function is called.
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/javatest...
> File
>
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelperTest.java
> (right):
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/javatest...
>
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelperTest.java:27:
> * Tests org.chromium.chrome.browser.webapps.AddToHomescreenHelper and it's C++
> counterpart.
> nit: it's -> its
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/shortcut_helper.h (right):
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
> chrome/browser/android/shortcut_helper.h:15: // from there via a JNI
> (Initialize) call and MUST BE DESTROYED via Destroy().
> Update the class description.  There's no Destroy() call anymore (e.g.)
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
> File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):
> 
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
> chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:5: #ifndef
> CHROME_BROWSER_ANDROID_WEBAPPS_ADD_TO_HOMESCREEN_DATA_FETCHER_H_
> Does it make sense to move these to the webapps/ directory, given that these
> aren't used _just_ for webapps?

Probably isn't the best answer but even I haven't fully fleshed out how these
components are interconnected - I have an idea but I guess it would be pretty
good for you, Mounir and I to all see the bigger picture. I'm working on a
design doc for splashscreen storage (quick FYI on that front - looks like SQLite
may acually be faster than SharedPreferences) and once that's done I'll create
an implementation doc.

I will fix your comments tomorrow morning :)

[Lalit Maganti, 2015-08-13 21:52:59]

On 2015/08/13 21:50:00, Lalit Maganti wrote:
> On 2015/08/13 21:43:15, dfalcantara wrote:
> > I get the gist of what you're doing, but you need to update the commit
message
> > to reflect what's really going on.
> > 
> > 1) Is there a implementation doc somewhere that describes how the code
reviews
> > you're sending are all interconnected?  I'm flying relatively blind on why
> > things are happening or moving around.
> > 
> > 2) You need to be extremely careful when copy-pasting files and splitting
> > classes apart.  Most of the comments I saw don't make sense anymore and need
> to
> > be updated.
> > 
> > 3) AddToHomescreeenHelper and ShortcutHelper sound like they should be doing
> the
> > same thing, name-wise, but I can't give you a better one to use without
> knowing
> > what's coming down the pipe.
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:1:
> > // Copyright 2014 The Chromium Authors. All rights reserved.
> > nit: 2015
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:24:
> > * logic to ShortcutHelper.
> > Update all comments to reflect the split and what they do now.  Also refer
to
> > the right classes.  May as well change valid references to "shortcut helper"
> to
> > "ShortcutHelper" while you're in here.
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java:17:
> > * This is a helper class to create shortcuts on the Android home screen.
> > Change the comment to better describe what this thing does post-split.
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java:84:
> > * @param userRequestedTitle Updated title for the shortcut.
> > nit: Not sure what an "Updated title for the shortcut" means without any
> context
> > from places where this function is called.
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/javatest...
> > File
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelperTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/android/javatest...
> >
>
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelperTest.java:27:
> > * Tests org.chromium.chrome.browser.webapps.AddToHomescreenHelper and it's
C++
> > counterpart.
> > nit: it's -> its
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
> > File chrome/browser/android/shortcut_helper.h (right):
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/shortcut_helper.h:15: // from there via a JNI
> > (Initialize) call and MUST BE DESTROYED via Destroy().
> > Update the class description.  There's no Destroy() call anymore (e.g.)
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
> > File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h
(right):
> > 
> >
>
https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
> > chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:5: #ifndef
> > CHROME_BROWSER_ANDROID_WEBAPPS_ADD_TO_HOMESCREEN_DATA_FETCHER_H_
> > Does it make sense to move these to the webapps/ directory, given that these
> > aren't used _just_ for webapps?
> 
> Probably isn't the best answer but even I haven't fully fleshed out how these
> components are interconnected - I have an idea but I guess it would be pretty
> good for you, Mounir and I to all see the bigger picture. I'm working on a
> design doc for splashscreen storage (quick FYI on that front - looks like
SQLite
> may acually be faster than SharedPreferences) and once that's done I'll create
> an implementation doc.
> 
> I will fix your comments tomorrow morning :)

And on point 3: essentially I want whatever the new ShortcutHelper class to
contain all the common code for Add To Homescreen and AppBanner and
AddToHomescreen to just contain the specific code for AddToHomescreen. These
names was what I felt best reflected what I was trying to do - maybe add some
indication of the common nature of the ShortcutHelper code in the class name?

[gone, 2015-08-14 00:00:30]

On 2015/08/13 21:52:59, Lalit Maganti wrote:
> Probably isn't the best answer but even I haven't fully fleshed out how these
> components are interconnected - I have an idea but I guess it would be pretty
> good for you, Mounir and I to all see the bigger picture. I'm working on a
> design doc for splashscreen storage (quick FYI on that front - looks like
> SQLite
> may acually be faster than SharedPreferences) and once that's done I'll create
> an implementation doc.

Hrm, SQLite seems like it'd be a bit overkill given that you wouldn't be
updating or
storing much data in it:
https://blograchita.wordpress.com/2013/05/26/difference-between-sqlite-and-sh...

> And on point 3: essentially I want whatever the new ShortcutHelper class to
> contain all the common code for Add To Homescreen and AppBanner and
> AddToHomescreen to just contain the specific code for AddToHomescreen. These
> names was what I felt best reflected what I was trying to do - maybe add some
> indication of the common nature of the ShortcutHelper code in the class name?

I guess the problem is that we've got a bunch of names for the same thing, which
sometimes
get reused for other things.

Fun history trivia so you understand why coming up with a name is more difficult
than
actually splitting apart the ShortcutHelper code:

* The implementation of standalone mode for Clank started out as "Application
Shortcuts",
  then "standalone mode", then eventually "webapp mode".

* Adding an application shortcut goes through the "Add to home screen" menu
item, but
  the menu item was originally added so that you could add shortcuts to your
home screen
  for opening specific web pages in the main Clank browser (application
shortcuts came
  many years later).  ShortcutHelper was expanded to account for both kinds of
shortcuts,
  which is how we've gotten to this CL.

* App banners adopted the language "Add to home screen" because of the menu
item.
  Problematically, app banners appear only for pages with manifest files (and
ignores
  pages which use only the old standalone mode meta tag), which makes it sound
like that
  phrase is used only when installing a web app.

* "App banners", "add to home screen", and "web apps" are all now treated as
synonyms,
  which doesn't make sense and makes its confusing to determine what people are
really
  talking about.

* Home screen shortcuts that open web pages are incorrectly called "bookmarks".
  In actuality, "bookmarks" are managed directly by Clank and have a separate
star
  item in the menu.

* The main user-facing interface to ShortcutHelper is the AddToHomescreenDialog,
which
  really should have been called the AddShortcutDialog or something.

Given all of that, I'm still having some trouble grasping what the difference
between
the two classes will be.  What other functions are going to be going into
ShortcutHelper,
for example?  Does it make sense to merge what's left of the ShortcutHelper with
the
BookmarkUtils?

[Lalit Maganti, 2015-08-19 10:53:42]

Previous patch seems to have made the refactoring a lot bigger but most files
are just affected by the BookmarkUtils -> ShortcutHelper name change. No
functional changes should occur in this CL.

[mlamouri (slow - plz ping), 2015-08-19 14:19:38]

still lgtm

[gone, 2015-08-19 18:49:17]

commit message:
componenets -> components

lgtm with the CL as is

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

https://chromiumcodereview.appspot.com/1288903002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:69:
public static final String REUSE_URL_MATCHING_TAB_ELSE_NEW_TAB =
while you're here, move this up with the other public static final Strings

https://chromiumcodereview.appspot.com/1288903002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:175:
Intent shortcutIntent, String title, Bitmap icon, String url) {
can you make the ordering of the parameters consistent with the one below?

https://chromiumcodereview.appspot.com/1288903002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:220: *
Creates an icon to be associated with this bookmark. If available, the touch
icon
bookmark -> shortcut

https://chromiumcodereview.appspot.com/1288903002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java
(right):

https://chromiumcodereview.appspot.com/1288903002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
I wonder if it makes sense to merge all of this into the AddToHomescreenDialog
class at some point.

https://chromiumcodereview.appspot.com/1288903002/diff/80001/chrome/browser/a...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://chromiumcodereview.appspot.com/1288903002/diff/80001/chrome/browser/a...
chrome/browser/android/chrome_jni_registrar.cc:254:
ShortcutHelper::RegisterShortcutHelper},
any reason why this moved down?  it fits on the line, doesn't it?

[Lalit Maganti, 2015-08-20 17:12:18]

As well as addressing the comments, I've renamed
AddToHomescreenDialogHelperObserver to Observer since it's an inner class so it
needs to reference AddToHomescreenDialogHelper anyway. 

Please let me know if this is not OK - I'm committing the change.

https://codereview.chromium.org/1288903002/diff/80001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/1288903002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:69:
public static final String REUSE_URL_MATCHING_TAB_ELSE_NEW_TAB =
On 2015/08/19 18:49:16, dfalcantara wrote:
> while you're here, move this up with the other public static final Strings

Done.

https://codereview.chromium.org/1288903002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java
(right):

https://codereview.chromium.org/1288903002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialogHelper.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/08/19 18:49:16, dfalcantara wrote:
> I wonder if it makes sense to merge all of this into the AddToHomescreenDialog
> class at some point.

Yes I think that would probably make a lot of sense. I'd probably do that after
splascreen lands though since it'll be clearer then if the merge makes sense
after the amount of code in this class increases.

https://codereview.chromium.org/1288903002/diff/80001/chrome/browser/android/...
File chrome/browser/android/chrome_jni_registrar.cc (right):

https://codereview.chromium.org/1288903002/diff/80001/chrome/browser/android/...
chrome/browser/android/chrome_jni_registrar.cc:254:
ShortcutHelper::RegisterShortcutHelper},
On 2015/08/19 18:49:16, dfalcantara wrote:
> any reason why this moved down?  it fits on the line, doesn't it?

Sorry this was a regression. I fixed this at some point based on your comment
previously but the change must have got lost :/ Yes it fits and I've put it
back.

[Lalit Maganti, 2015-08-20 17:12:48]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-20 17:12:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1288903002/#ps100001
(title: "Fix comments on review + change AddToHomescreenDialogHelperObserver ->
Observer")

[commit-bot: I haz the power, 2015-08-20 17:13:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288903002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288903002/100001

[commit-bot: I haz the power, 2015-08-20 17:29:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-20 17:29:23]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Lalit Maganti, 2015-08-20 17:32:57]

The CQ bit was checked by lalitm@google.com

[commit-bot: I haz the power, 2015-08-20 17:34:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288903002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288903002/100001

[Lalit Maganti, 2015-08-20 17:44:03]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-20 17:44:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1288903002/#ps120001
(title: "Fix build files")

[commit-bot: I haz the power, 2015-08-20 17:44:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288903002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288903002/120001

[commit-bot: I haz the power, 2015-08-20 18:02:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-20 18:02:09]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Lalit Maganti, 2015-08-20 19:16:35]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-20 19:16:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1288903002/#ps140001
(title: "Fix build")

[commit-bot: I haz the power, 2015-08-20 19:17:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288903002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288903002/140001

[Lalit Maganti, 2015-08-20 20:44:21]

The CQ bit was checked by lalitm@google.com

[Lalit Maganti, 2015-08-20 20:44:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1288903002/#ps200001
(title: "Fix test file name")

[commit-bot: I haz the power, 2015-08-20 20:44:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1288903002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1288903002/200001

[commit-bot: I haz the power, 2015-08-20 22:06:09]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-20 22:06:47]

Patchset 11 (id:??) landed as
https://crrev.com/870920edb6dd146b86d84d99cbeee1c47c76ce53
Cr-Commit-Position: refs/heads/master@{#344595}

[Lalit Maganti, 2015-08-26 13:57:08]

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java
(right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/08/13 at 21:43:15, dfalcantara wrote:
> nit: 2015

Done in https://codereview.chromium.org/1321463002

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenDialog.java:24:
* logic to ShortcutHelper.
On 2015/08/13 at 21:43:15, dfalcantara wrote:
> Update all comments to reflect the split and what they do now.  Also refer to
the right classes.  May as well change valid references to "shortcut helper" to
"ShortcutHelper" while you're in here.

Done in https://codereview.chromium.org/1321463002

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java
(right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java:17:
* This is a helper class to create shortcuts on the Android home screen.
On 2015/08/13 at 21:43:15, dfalcantara wrote:
> Change the comment to better describe what this thing does post-split.

Done in https://codereview.chromium.org/1321463002

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelper.java:84:
* @param userRequestedTitle Updated title for the shortcut.
On 2015/08/13 at 21:43:15, dfalcantara wrote:
> nit: Not sure what an "Updated title for the shortcut" means without any
context from places where this function is called.

Done in https://codereview.chromium.org/1321463002

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelperTest.java
(right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/AddToHomescreenHelperTest.java:27:
* Tests org.chromium.chrome.browser.webapps.AddToHomescreenHelper and it's C++
counterpart.
On 2015/08/13 at 21:43:15, dfalcantara wrote:
> nit: it's -> its

Done in https://codereview.chromium.org/1321463002

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
File chrome/browser/android/shortcut_helper.h (right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
chrome/browser/android/shortcut_helper.h:15: // from there via a JNI
(Initialize) call and MUST BE DESTROYED via Destroy().
On 2015/08/13 at 21:43:15, dfalcantara wrote:
> Update the class description.  There's no Destroy() call anymore (e.g.)

Done in https://codereview.chromium.org/1321463002

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h (right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h:5: #ifndef
CHROME_BROWSER_ANDROID_WEBAPPS_ADD_TO_HOMESCREEN_DATA_FETCHER_H_
On 2015/08/13 at 21:43:15, dfalcantara wrote:
> Does it make sense to move these to the webapps/ directory, given that these
aren't used _just_ for webapps?

Since now this is exclusively used by the dialog it does make sense.

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapps/add_to_homescreen_helper.h (right):

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_helper.h:25: // ShortcutHelper
is the C++ counterpart of org.chromium.chrome.browser's
On 2015/08/13 at 20:54:28, dfalcantara wrote:
> you didn't change any of the comments to change the ShortcutHelper references

Done in https://codereview.chromium.org/1321463002

https://codereview.chromium.org/1288903002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapps/add_to_homescreen_helper.h:25: // ShortcutHelper
is the C++ counterpart of org.chromium.chrome.browser's
On 2015/08/13 at 20:54:28, dfalcantara wrote:
> you didn't change any of the comments to change the ShortcutHelper references

Done in later patchset in this series.