[Mac] Rebuild app shims when they fail to dyload Chrome Framework.

[Mac] Rebuild app shims when they fail to dyload Chrome Framework.

This also changes app_mode_loader to start Chrome with --app-shim-error
only when it was started by Chrome or if it is the app_list shim. When
started by the user, it launches Chrome with --app-id. This simplifies
how Chrome handles --app-shim-error. It does not need to load the
profile or app extension, and rebuilding the shim does not block
launching the app.

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

[tapted, 2014-05-05 07:02:01]

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:124: if (ChromeAppModeStart) {
nit: doesn't need curlies

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:137: // Just launch the app.
nit: maybe clarify why this branching exists and that it will ultimately come
back to the above case. E.g. ~"If the shim was launched directly (instead of by
Chrome), first ask Chrome to launch the app. Chrome will launch the shim again
(in parallel). The same error will occur and be handled above. This approach
makes the launch more responsive/robust/othereasons.."

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:142: if (!base::LaunchProcess(command_line,
base::LaunchOptions(), NULL)) {
why was this change needed? (i.e. changing from
base::mac::OpenApplicationWithPath)

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:148: return 0;
Maybe really unlikely.. but is there a potential race condition here if this
process stays alive? i.e. Could the shim re-launch be blocked due to the way
LSOpenApplication works, e.g. by putting a big sleep before this return?

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:155: CommandLine::Init(argc, argv);
Hm - this will be called again at ChromeAppModeStart and it will "fail" (return
false). But of course we can't remove that second call or it will break working
shims. Just pondering whether there are any downsides to this, but I think it's
fine.

https://codereview.chromium.org/265163006/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/265163006/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_service.h:107: virtual void CreateShortcut()
= 0;
multi-nit: you'll need to update comments like

  // AppListServiceImpl overrides:
  virtual void CreateShortcut() OVERRIDE;

in app_list_service_{mac,linux,win}.h (i.e. either to say "AppListService
overrides", or just to merge the fn dec into a chunk above)

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:102: typedef base::Callback<void(const
web_app::ShortcutInfo&,
nit: this should have a comment

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app_mac.h (right):

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.h:12: #include "base/command_line.h"
nit: forward declare?

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:896: if
(bundle_base_name.length() < 40) {
40 -> constant?

But all the returns below here `return true;` Could you just post a task to read
the plist?

Something like

  base::FilePath shim_path =
      command_line.GetSwitchValuePath(app_mode::kAppShimError);
  base::PostTaskAndReplyWithResult(
      BrowserThread::GetBlockingPool(),
      FROM_HERE,
      base::Bind(BuildShortcutInfoFromBundle, shim_path),
      base::Bind(RebuildAppAndLaunch, shim_path));
  return true;
}

  void RebuildAppAndLaunch(const base::FilePath& app_data_path,
      const ShortcutInfo& info) {
    if (info.extension_id ==  app_mode::kAppListModeId) {
      app_list_service->CreateShortcut();
      app_list_service->Show();
      return;
    }
    PostTaskAndReply(
        ..
        Bind(UpdatePlatformShortcuts(app_data_path, "", info,
FileHandlersInfo()),
        Bind(LanuchShim, ..));
  }


The string stuff just feels a little weird. E.g. we go through some extra hoops
to handle someone moving a shim (beyond just finder drag & drop creating an
alias). And in that case, there's a higher chance the shim has been renamed from
the `Profile extension_id` pattern as well

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:908: std::string app_id =
parts.back();
hypothetically.... will this crash if bundle_base_name was an empty string?

[jackhou1, 2014-05-06 04:47:58]

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:124: if (ChromeAppModeStart) {
On 2014/05/05 07:02:01, tapted wrote:
> nit: doesn't need curlies

Done.

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:137: // Just launch the app.
On 2014/05/05 07:02:01, tapted wrote:
> nit: maybe clarify why this branching exists and that it will ultimately come
> back to the above case. E.g. ~"If the shim was launched directly (instead of
by
> Chrome), first ask Chrome to launch the app. Chrome will launch the shim again
> (in parallel). The same error will occur and be handled above. This approach
> makes the launch more responsive/robust/othereasons.."

Done.

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:142: if (!base::LaunchProcess(command_line,
base::LaunchOptions(), NULL)) {
On 2014/05/05 07:02:01, tapted wrote:
> why was this change needed? (i.e. changing from
> base::mac::OpenApplicationWithPath)

Updated the comment. LSOpenApplication doesn't pass the command line properly.

https://codereview.chromium.org/265163006/diff/1/chrome/app/app_mode_loader_m...
chrome/app/app_mode_loader_mac.mm:148: return 0;
On 2014/05/05 07:02:01, tapted wrote:
> Maybe really unlikely.. but is there a potential race condition here if this
> process stays alive? i.e. Could the shim re-launch be blocked due to the way
> LSOpenApplication works, e.g. by putting a big sleep before this return?

Yeah there is, but I don't think it's worth trying to handle. Chrome would have
to start up, connect to the process singleton, and launch the app before this
shuts down.

https://codereview.chromium.org/265163006/diff/1/chrome/browser/ui/app_list/a...
File chrome/browser/ui/app_list/app_list_service.h (right):

https://codereview.chromium.org/265163006/diff/1/chrome/browser/ui/app_list/a...
chrome/browser/ui/app_list/app_list_service.h:107: virtual void CreateShortcut()
= 0;
On 2014/05/05 07:02:01, tapted wrote:
> multi-nit: you'll need to update comments like
> 
>   // AppListServiceImpl overrides:
>   virtual void CreateShortcut() OVERRIDE;
> 
> in app_list_service_{mac,linux,win}.h (i.e. either to say "AppListService
> overrides", or just to merge the fn dec into a chunk above)

Done.

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app.h:102: typedef base::Callback<void(const
web_app::ShortcutInfo&,
On 2014/05/05 07:02:01, tapted wrote:
> nit: this should have a comment

Done.

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app_mac.h (right):

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.h:12: #include "base/command_line.h"
On 2014/05/05 07:02:01, tapted wrote:
> nit: forward declare?

Done.

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
File chrome/browser/web_applications/web_app_mac.mm (right):

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:896: if
(bundle_base_name.length() < 40) {
On 2014/05/05 07:02:01, tapted wrote:
> 40 -> constant?
> 
> But all the returns below here `return true;` Could you just post a task to
read
> the plist?
> 
> Something like
> 
>   base::FilePath shim_path =
>       command_line.GetSwitchValuePath(app_mode::kAppShimError);
>   base::PostTaskAndReplyWithResult(
>       BrowserThread::GetBlockingPool(),
>       FROM_HERE,
>       base::Bind(BuildShortcutInfoFromBundle, shim_path),
>       base::Bind(RebuildAppAndLaunch, shim_path));
>   return true;
> }
> 
>   void RebuildAppAndLaunch(const base::FilePath& app_data_path,
>       const ShortcutInfo& info) {
>     if (info.extension_id ==  app_mode::kAppListModeId) {
>       app_list_service->CreateShortcut();
>       app_list_service->Show();
>       return;
>     }
>     PostTaskAndReply(
>         ..
>         Bind(UpdatePlatformShortcuts(app_data_path, "", info,
> FileHandlersInfo()),
>         Bind(LanuchShim, ..));
>   }
> 
> 
> The string stuff just feels a little weird. E.g. we go through some extra
hoops
> to handle someone moving a shim (beyond just finder drag & drop creating an
> alias). And in that case, there's a higher chance the shim has been renamed
from
> the `Profile extension_id` pattern as well

Done.

Good idea. It still needs to get the Profile* and Extension*, and load the icon
though.

https://codereview.chromium.org/265163006/diff/1/chrome/browser/web_applicati...
chrome/browser/web_applications/web_app_mac.mm:908: std::string app_id =
parts.back();
On 2014/05/05 07:02:01, tapted wrote:
> hypothetically.... will this crash if bundle_base_name was an empty string?

Removed.

[tapted, 2014-05-06 07:13:23]

lgtm

https://codereview.chromium.org/265163006/diff/40001/chrome/app/app_mode_load...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/265163006/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:166: info.minor_version =
app_mode::kCurrentChromeAppModeInfoMinorVersion;
we should increment this again, for the bulk update.

https://codereview.chromium.org/265163006/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/win/app_list_service_win.h (right):

https://codereview.chromium.org/265163006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/win/app_list_service_win.h:27: virtual void
DismissAppList() OVERRIDE;
I think there's a rebase problem here -- some of these no longer exist on
AppListServiceWin

https://codereview.chromium.org/265163006/diff/40001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/265163006/diff/40001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app.cc:174: static const char* kCrxAppPrefix
= "_crx_";
nit: (since it's showing up in the diff) should this be `const char
kCrxAppPrefix[]` (instead of char*) like all the other strings?

[jackhou1, 2014-05-06 08:38:16]

https://codereview.chromium.org/265163006/diff/40001/chrome/app/app_mode_load...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/265163006/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:166: info.minor_version =
app_mode::kCurrentChromeAppModeInfoMinorVersion;
On 2014/05/06 07:13:24, tapted wrote:
> we should increment this again, for the bulk update.

Done.

https://codereview.chromium.org/265163006/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/win/app_list_service_win.h (right):

https://codereview.chromium.org/265163006/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/win/app_list_service_win.h:27: virtual void
DismissAppList() OVERRIDE;
On 2014/05/06 07:13:24, tapted wrote:
> I think there's a rebase problem here -- some of these no longer exist on
> AppListServiceWin

Done.

https://codereview.chromium.org/265163006/diff/40001/chrome/browser/web_appli...
File chrome/browser/web_applications/web_app.cc (right):

https://codereview.chromium.org/265163006/diff/40001/chrome/browser/web_appli...
chrome/browser/web_applications/web_app.cc:174: static const char* kCrxAppPrefix
= "_crx_";
On 2014/05/06 07:13:24, tapted wrote:
> nit: (since it's showing up in the diff) should this be `const char
> kCrxAppPrefix[]` (instead of char*) like all the other strings?

Done.

[jackhou1, 2014-05-06 08:40:27]

thakis, please review for OWNERS:

chrome/app/app_mode_loader_mac.mm
chrome/browser/ui/startup/startup_browser_creator.cc
chrome/common/mac/

[Nico, 2014-05-07 23:45:43]

Is it possible to test this? How large is a 32bit app shim binary? Maybe we can
just check one into testdata?

https://codereview.chromium.org/265163006/diff/80001/chrome/app/app_mode_load...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/265163006/diff/80001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:147: // application is already running.
If chrome's running already, this depends on the profile lock causing chrome to
somehow passing the commandline args to the already-running instance? How
exactly does the arg relaying work?

[jackhou1, 2014-06-11 03:01:36]

tapted, PTAL at Patch 7

Main changes:
- added 32-bit shim built with this patch,
- added test to AppShimInteractiveTest that launches the 32-bit shim,
- added kLaunchedAfterRebuild to prevent endless loop if rebuild doesn't fix
problem.

[tapted, 2014-06-11 03:32:44]

https://codereview.chromium.org/265163006/diff/120001/apps/app_shim/app_shim_...
File apps/app_shim/app_shim_interactive_uitest_mac.mm (right):

https://codereview.chromium.org/265163006/diff/120001/apps/app_shim/app_shim_...
apps/app_shim/app_shim_interactive_uitest_mac.mm:307: base::FilePath
shim_path_32 =
Should this actually try to pick a shim that is the inverse of the current
build?

i.e. If this gets compiled for 32-bit the test as it is should just bail out,
since a 32-bit build can't make a 64-bit shim to complete the test.

Bailing out is an option, but we could add a prebuilt 64-bit version and test
someone "upgrading" from a 64-bit chrome to 32-bit, which happens a lot on
dev/canary channels.

https://codereview.chromium.org/265163006/diff/120001/apps/app_shim/app_shim_...
apps/app_shim/app_shim_interactive_uitest_mac.mm:374:
EXPECT_TRUE(HasAppShimHost(profile(), app->id()));
Is there a non-flaky way to do an EXPECT_FALSE version of this between
launched_listener.Wait and observer.Wait? Or possibly before
launched_listener.Wait by hooking into AppLifetimeMonitor::Observer. I think
that would help verify that the shim launch did actually fail the first time and
the app was launched without a shimhost via --app-shim-error

https://codereview.chromium.org/265163006/diff/120001/chrome/test/data/app_sh...
File chrome/test/data/app_shim/app_shim_32_bit/Contents/PkgInfo (right):

https://codereview.chromium.org/265163006/diff/120001/chrome/test/data/app_sh...
chrome/test/data/app_shim/app_shim_32_bit/Contents/PkgInfo:1: APPL????
we might want to dcommit these... CQ could do Weird Things

[jackhou1, 2014-06-13 09:47:49]

https://codereview.chromium.org/265163006/diff/120001/apps/app_shim/app_shim_...
File apps/app_shim/app_shim_interactive_uitest_mac.mm (right):

https://codereview.chromium.org/265163006/diff/120001/apps/app_shim/app_shim_...
apps/app_shim/app_shim_interactive_uitest_mac.mm:307: base::FilePath
shim_path_32 =
On 2014/06/11 03:32:43, tapted wrote:
> Should this actually try to pick a shim that is the inverse of the current
> build?
> 
> i.e. If this gets compiled for 32-bit the test as it is should just bail out,
> since a 32-bit build can't make a 64-bit shim to complete the test.
> 
> Bailing out is an option, but we could add a prebuilt 64-bit version and test
> someone "upgrading" from a 64-bit chrome to 32-bit, which happens a lot on
> dev/canary channels.

Oh yeah, I forgot to ifdef this out. I don't think testing the 64-to-32 case
would add much, all of the code is the same, it would just test that a 64-bit
shim fails to dyload a 32-bit framework.

https://codereview.chromium.org/265163006/diff/120001/apps/app_shim/app_shim_...
apps/app_shim/app_shim_interactive_uitest_mac.mm:374:
EXPECT_TRUE(HasAppShimHost(profile(), app->id()));
On 2014/06/11 03:32:43, tapted wrote:
> Is there a non-flaky way to do an EXPECT_FALSE version of this between
> launched_listener.Wait and observer.Wait? Or possibly before
> launched_listener.Wait by hooking into AppLifetimeMonitor::Observer. I think
> that would help verify that the shim launch did actually fail the first time
and
> the app was launched without a shimhost via --app-shim-error

It should be non-flaky to do those waits sequentially, there are many sequential
steps on both the UI and FILE threads involved in the rebuild.

https://codereview.chromium.org/265163006/diff/120001/chrome/test/data/app_sh...
File chrome/test/data/app_shim/app_shim_32_bit/Contents/PkgInfo (right):

https://codereview.chromium.org/265163006/diff/120001/chrome/test/data/app_sh...
chrome/test/data/app_shim/app_shim_32_bit/Contents/PkgInfo:1: APPL????
On 2014/06/11 03:32:43, tapted wrote:
> we might want to dcommit these... CQ could do Weird Things

Yeah, the debug build is 2.5MB, which git cl refuses to upload. Changed this to
an official release build (150KB) plus dSYM (0.8MB). I'll watch it and be
prepared to revert if anything goes wrong.

[tapted, 2014-06-13 11:15:58]

lgtm

https://codereview.chromium.org/265163006/diff/160001/chrome/common/mac/app_m...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/265163006/diff/160001/chrome/common/mac/app_m...
chrome/common/mac/app_mode_chrome_locator.mm:64: // chrome::kFrameworkName looks
like "$PRODUCT_STRING Framework.framework".
nit: update this comment. Probably still worth mentioning kFrameworkName, but it
should highlight why it isn't used now (i.e. so that a single shim binary can
talk to either an official or chromium build, by updating its plist).

https://codereview.chromium.org/265163006/diff/160001/chrome/test/data/app_sh...
File chrome/test/data/app_shim/app_shim_32_bit.app/Contents/Info.plist (right):

https://codereview.chromium.org/265163006/diff/160001/chrome/test/data/app_sh...
chrome/test/data/app_shim/app_shim_32_bit.app/Contents/Info.plist:49:
<key>LSFileQuarantineEnabled</key>
Should we remove this manually to try to avoid those subtle kernel bugs on 10.7?

[jackhou1, 2014-06-14 10:09:33]

https://codereview.chromium.org/265163006/diff/160001/chrome/common/mac/app_m...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/265163006/diff/160001/chrome/common/mac/app_m...
chrome/common/mac/app_mode_chrome_locator.mm:64: // chrome::kFrameworkName looks
like "$PRODUCT_STRING Framework.framework".
On 2014/06/13 11:15:58, tapted wrote:
> nit: update this comment. Probably still worth mentioning kFrameworkName, but
it
> should highlight why it isn't used now (i.e. so that a single shim binary can
> talk to either an official or chromium build, by updating its plist).

Done.

https://codereview.chromium.org/265163006/diff/160001/chrome/test/data/app_sh...
File chrome/test/data/app_shim/app_shim_32_bit.app/Contents/Info.plist (right):

https://codereview.chromium.org/265163006/diff/160001/chrome/test/data/app_sh...
chrome/test/data/app_shim/app_shim_32_bit.app/Contents/Info.plist:49:
<key>LSFileQuarantineEnabled</key>
On 2014/06/13 11:15:58, tapted wrote:
> Should we remove this manually to try to avoid those subtle kernel bugs on
10.7?

Argh, yeah I forgot to patch in latest version of that CL.

[jackhou1, 2014-06-16 21:31:23]

thakis, PTAL

On 2014/05/07 23:45:43, Nico (away) wrote:
> Is it possible to test this? How large is a 32bit app shim binary? Maybe we
can
> just check one into testdata?
> 
>
https://codereview.chromium.org/265163006/diff/80001/chrome/app/app_mode_load...
> File chrome/app/app_mode_loader_mac.mm (right):

Added some app shim testing in https://codereview.chromium.org/316493002/. Also
added a test with a checked-in 32-bit shim in this CL. 
A release build shim is 150KB, I've also included the dSYM (800KB) in case it
needs to be debugged at some point. By comparison a debug build shim is 2.5MB.

>
https://codereview.chromium.org/265163006/diff/80001/chrome/app/app_mode_load...
> chrome/app/app_mode_loader_mac.mm:147: // application is already running.
> If chrome's running already, this depends on the profile lock causing chrome
to
> somehow passing the commandline args to the already-running instance? How
> exactly does the arg relaying work?

On Mac, it now works the same way as Linux
(https://codereview.chromium.org/218883008/).
I.e. there's an IPC socket that the singleton process listens on.

[Nico, 2014-06-16 22:31:00]

lgtm

Thanks for adding a test!

800kB isn't small, but the test binary will probably not change often (or ever),
so I think it's ok.

https://codereview.chromium.org/265163006/diff/180001/chrome/app/app_mode_loa...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/265163006/diff/180001/chrome/app/app_mode_loa...
chrome/app/app_mode_loader_mac.mm:138: // the problem, so don't try again.
Should we have an UMA stat that counts how often this happens? (doesn't have to
be in this cl)

[jackhou1, 2014-06-16 22:46:41]

https://codereview.chromium.org/265163006/diff/180001/chrome/app/app_mode_loa...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/265163006/diff/180001/chrome/app/app_mode_loa...
chrome/app/app_mode_loader_mac.mm:138: // the problem, so don't try again.
On 2014/06/16 22:31:00, Nico (away) wrote:
> Should we have an UMA stat that counts how often this happens? (doesn't have
to
> be in this cl)

Yeah, this is probably worthwhile. I'd like to be able to get a bit more
information out of it though, like whether the failure was actually due to
having different bit-ness.

[jackhou1, 2014-06-18 08:01:40]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-06-18 08:04:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/265163006/200001

[jackhou1, 2014-06-18 12:02:21]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-06-18 12:03:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/265163006/220001

[jackhou1, 2014-06-18 12:17:49]

CQ is failing on checkperms because it can't commit executables. Pulling the
test out into a separate CL to dcommit.
https://codereview.chromium.org/345493004/

[jackhou1, 2014-06-18 12:51:10]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-06-18 12:52:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/265163006/240001

[commit-bot: I haz the power, 2014-06-18 18:26:41]

Change committed as 278126