Add Chrome To Mobile Service and Views Page Action.

Add Chrome To Mobile Service and Views Page Action.
Implements the Chrome To Mobile extension in Chrome.

List the user's mobile devices via the Cloud Print server.
Add a page action icon when the service reports 1+ devices.
Add a bubble to send the current page URL / MHTML snapshot.

The bubble shows a radio group for multiple devices.
 (or it shows a single device as part of the title label)
The bubble also shows a checkbox to send an offline copy.
Send URLFetcher requests to GET/POST the URL/Snapshot.
The bubble shows "Sending..."/"Sent"/ error request status.

BUG=102709
TEST=New Chrome To Mobile bubble works as expected :)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126343

[msw, 2012-03-10 00:40:33]

PTAL, consider my TODOs, and suggest reviewers; thanks!
Lots may be wrong, advice for major changes welcome!

sky: chrome/browser/ui, chrome/app, and ui/base
gene, scottbyer, abodenha: chrome_to_mobile_service[_factory],
    chrome/common/cloud_print, and chrome/service/cloud_print
nyquist, jcivelli: as appropriate, maybe just the service?

[sky, 2012-03-11 21:41:22]

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/app/generate...
chrome/app/generated_resources.grd:6953: +        Also send copy for offline
viewing (<ph name="PAGE_SIZE">$1<ex>111kB</ex></ph>)
Should this be 'Also send a copy' ?

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
chrome/browser/ui/browser.h:579: void ChromeToMobile();
ChromeToMobile is a confusing name for this. How about SendPagetoMobileDevice?
My suggestion applies to all places you're using chrometomobile.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
chrome/browser/ui/browser_window.h:218: virtual void ShowChromeToMobileBubble()
{}
make pure virtual.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
File chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:121:
content::BrowserThread::PostBlockingPoolTask(FROM_HERE,
I'm not familiar with this, does it black the UI thread?

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:151:
progress_label_->SetText(text.append(ASCIIToUTF16(".")));
I suspect that adding a '.' to the end of the string isn't always the right
thing for some locales. Even so, have you talked with the UX guys about this ui?
For example, I would rather have send return immediately but push error messages
to either an infobar or some other later ui.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:300:
RemoveAllChildViews(true);
Reset the fields that point to views this deletes.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:320:
progress_animation_->set_timer_interval(
I think you want SetDuration and don't add set_timer_interval.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
File chrome/browser/ui/views/chrome_to_mobile_bubble_view.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:37: // Don't create a
ChromeToMobileBubbleView directly, use ShowBubble method.
nit: update comment as you can't really create a ChromeToMobilebubbleview as
it's private.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:46: virtual
~ChromeToMobileBubbleView();
destructor before static emthods.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:64: // Called upon
generation of the page's MHTML snapshot.
nit: typically overriden virtual methods are last in a section.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:92: DeviceMap
mobile_map_;
Add a description of mobile_map_ and selected_mobile_.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:93:
base::DictionaryValue* selected_mobile_;
Initialize this to NULL.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:102: size_t
progress_periods_count_;
Add a description of this.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/chrome_to_mobile_view.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/chrome_to_mobile_view.h:23: explicit
ChromeToMobileView(LocationBarView* location_bar_view,
no explicit.

[msw, 2012-03-12 10:17:18]

I'm looking into the tryjob failures, but don't have any leads.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/app/generate...
chrome/app/generated_resources.grd:6953: +        Also send copy for offline
viewing (<ph name="PAGE_SIZE">$1<ex>111kB</ex></ph>)
On 2012/03/11 21:41:22, sky wrote:
> Should this be 'Also send a copy' ?

Done.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
chrome/browser/ui/browser.h:579: void ChromeToMobile();
On 2012/03/11 21:41:22, sky wrote:
> ChromeToMobile is a confusing name for this. How about SendPagetoMobileDevice?
> My suggestion applies to all places you're using chrometomobile.

Renamed ShowChromeToMobileBubble, describing the function's action.
I was told the feature is "Chrome To Mobile" and tried to name consistently.
I'll rename the 200+ ChromeToMobile instances if we have an agreed upon name.
SendPagetoMobileDevice seems a little long; maybe SharePage? Thoughts?

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
File chrome/browser/ui/browser_window.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
chrome/browser/ui/browser_window.h:218: virtual void ShowChromeToMobileBubble()
{}
On 2012/03/11 21:41:22, sky wrote:
> make pure virtual.

Done (this implied adding several NOTIMPLEMENTED OVERRIDEs).

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
File chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:121:
content::BrowserThread::PostBlockingPoolTask(FROM_HERE,
On 2012/03/11 21:41:22, sky wrote:
> I'm not familiar with this, does it black the UI thread?

Nope, See the decl; this is the new FILE thread pattern.
http://code.google.com/codesearch#OAMlx_jo-ck/src/content/public/browser/brow...
You can also see Brett's chromium-dev email from January 18th:
"The FILE thread is dead, long live the blocking pool"
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:151:
progress_label_->SetText(text.append(ASCIIToUTF16(".")));
On 2012/03/11 21:41:22, sky wrote:
> I suspect that adding a '.' to the end of the string isn't always the right
> thing for some locales. Even so, have you talked with the UX guys about this
ui?
> For example, I would rather have send return immediately but push error
messages
> to either an infobar or some other later ui.

I'm now using multiple IDS messages for internationalization.
I sent mail with your suggestion, I unsuccessfully suggested this earlier.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:300:
RemoveAllChildViews(true);
On 2012/03/11 21:41:22, sky wrote:
> Reset the fields that point to views this deletes.

Done.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:320:
progress_animation_->set_timer_interval(
On 2012/03/11 21:41:22, sky wrote:
> I think you want SetDuration and don't add set_timer_interval.

Done. Would a RepeatingTimer be better than an Animation here?

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
File chrome/browser/ui/views/chrome_to_mobile_bubble_view.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:37: // Don't create a
ChromeToMobileBubbleView directly, use ShowBubble method.
On 2012/03/11 21:41:22, sky wrote:
> nit: update comment as you can't really create a ChromeToMobilebubbleview as
> it's private.

Good point, removed that sentence.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:46: virtual
~ChromeToMobileBubbleView();
On 2012/03/11 21:41:22, sky wrote:
> destructor before static emthods.

Done.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:64: // Called upon
generation of the page's MHTML snapshot.
On 2012/03/11 21:41:22, sky wrote:
> nit: typically overriden virtual methods are last in a section.

Moot here with latest code, but thanks for the tip.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:92: DeviceMap
mobile_map_;
On 2012/03/11 21:41:22, sky wrote:
> Add a description of mobile_map_ and selected_mobile_.

Done.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:93:
base::DictionaryValue* selected_mobile_;
On 2012/03/11 21:41:22, sky wrote:
> Initialize this to NULL.

Done.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.h:102: size_t
progress_periods_count_;
On 2012/03/11 21:41:22, sky wrote:
> Add a description of this.

Done.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/chrome_to_mobile_view.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/chrome_to_mobile_view.h:23: explicit
ChromeToMobileView(LocationBarView* location_bar_view,
On 2012/03/11 21:41:22, sky wrote:
> no explicit.

Done.

[sky, 2012-03-12 15:28:12]

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
chrome/browser/ui/browser.h:579: void ChromeToMobile();
On 2012/03/12 10:17:18, msw wrote:
> On 2012/03/11 21:41:22, sky wrote:
> > ChromeToMobile is a confusing name for this. How about
SendPagetoMobileDevice?
> > My suggestion applies to all places you're using chrometomobile.
> 
> Renamed ShowChromeToMobileBubble, describing the function's action.
> I was told the feature is "Chrome To Mobile" and tried to name consistently.
> I'll rename the 200+ ChromeToMobile instances if we have an agreed upon name.
> SendPagetoMobileDevice seems a little long; maybe SharePage? Thoughts?

SharePage is too vague. My vote stands, but I'll deal with what you have if that
is how we are exposing the feature to end users.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
File chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/v...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:320:
progress_animation_->set_timer_interval(
On 2012/03/12 10:17:18, msw wrote:
> On 2012/03/11 21:41:22, sky wrote:
> > I think you want SetDuration and don't add set_timer_interval.
> 
> Done. Would a RepeatingTimer be better than an Animation here?

Either one works, but we typically use animation for this sort of thing.

[msw, 2012-03-12 22:11:46]

Please take a look asap. I'm fixing trybot failures, and I'll gladly incorporate
any feedback on older patch sets into the latest.

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/9443007/diff/40001/chrome/browser/ui/b...
chrome/browser/ui/browser.h:579: void ChromeToMobile();
On 2012/03/12 15:28:12, sky wrote:
> On 2012/03/12 10:17:18, msw wrote:
> > On 2012/03/11 21:41:22, sky wrote:
> > > ChromeToMobile is a confusing name for this. How about
> SendPagetoMobileDevice?
> > > My suggestion applies to all places you're using chrometomobile.
> > 
> > Renamed ShowChromeToMobileBubble, describing the function's action.
> > I was told the feature is "Chrome To Mobile" and tried to name consistently.
> > I'll rename the 200+ ChromeToMobile instances if we have an agreed upon
name.
> > SendPagetoMobileDevice seems a little long; maybe SharePage? Thoughts?
> 
> SharePage is too vague. My vote stands, but I'll deal with what you have if
that
> is how we are exposing the feature to end users.

As I sent offline, we'll stick with Chrome To Mobile for now, and I'll
mass-rename later if appropriate.

[sky, 2012-03-12 23:06:27]

LGTM

[Scott Byer, 2012-03-12 23:26:23]

LGTM with a couple of include order / lint issues.

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:13: #include
"chrome/common/cloud_print/cloud_print_helpers.h"
Put in alphabetical order.

[nyquist, 2012-03-13 00:52:23]

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:201: content::URLFetcher* submit_url
= CreateRequest(data);
Nit: This naming seems odd to me. submit_request?

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:241: GURL auth_url =
cloud_print::GetUrlForGetAuthCode(
Dead store. This doesn't seem to be used anywhere.

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:274: mobiles_.clear();
Updating this list directly means that clients might get an empty list (or
partly filled), since there seems to be no thread safety between
HandleSearchResponse and retreiving the list of mobiles_.
Could we somewhere make it clear that this service can only be called from the
browser thread (if that is the case)? Or is that implicit given something else?

[msw, 2012-03-13 02:39:14]

Addressed comments and additional lints. I plan to land with green trybots, but
will gladly take additional feedback for this CL and followup CLs.

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:13: #include
"chrome/common/cloud_print/cloud_print_helpers.h"
On 2012/03/12 23:26:23, Scott Byer wrote:
> Put in alphabetical order.

Done.

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:201: content::URLFetcher* submit_url
= CreateRequest(data);
On 2012/03/13 00:52:23, nyquist wrote:
> Nit: This naming seems odd to me. submit_request?

Talked offline: submit_url corresponds with submit_snapshot.

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:241: GURL auth_url =
cloud_print::GetUrlForGetAuthCode(
On 2012/03/13 00:52:23, nyquist wrote:
> Dead store. This doesn't seem to be used anywhere.

Awesome catch! This let me revert some CloudPrint changes:
GetUrlForGetAuthCode, kDefaultCloudPrintOAuthClientId, & GenerateProxyId.

http://codereview.chromium.org/9443007/diff/55002/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:274: mobiles_.clear();
On 2012/03/13 00:52:23, nyquist wrote:
> Updating this list directly means that clients might get an empty list (or
> partly filled), since there seems to be no thread safety between
> HandleSearchResponse and retreiving the list of mobiles_.
> Could we somewhere make it clear that this service can only be called from the
> browser thread (if that is the case)? Or is that implicit given something
else?

I'll gladly take additional thread safety advice, but this should be ok.

I added a precautionary UI thread DCHECK above, ensuring the list is built on
the UI thread (should be the case as the URLFetcher requests are created and
sent by UI inits and updates, and URLFetcher calls back handlers on the same
thread, see doc). |mobiles()| is accessed via LocationBarView::Init|Update and
ChromeToMobileBubbleView::Init; these are also on the UI thread.

I also changed the in-place list building to use a local temp vector and
assignment, to avoid (but not resolve) potential simultaneous thread access in
future iterations of the code.

[commit-bot: I haz the power, 2012-03-13 04:51:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/9443007/54057

[commit-bot: I haz the power, 2012-03-13 06:08:49]

Change committed as 126343